Christian :Biesinger (don't email me, ping me on IRC) Reporter
	Description • 18 years ago

I'd like to eventually kill the NS_*_CAST macros because they look awful, have no syntax highlighting in editors, and all compilers really should support the "new" casts.

Per the tinderbox log at:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Ports/1155658185.12836.gz&fulltext=1
Sun's compiler supports them

Per my own testing, HP aCC HP ANSI C++ B3910B A.03.33 supports that too

So does gcc version 2.95.4 20011002 (Debian prerelease)

(The testing I did was compile the same program that the reinterpret_cast configure test compiles)

So as a first step I'd like to unconditionally define the macros as the new-style casts in nscore.h and see if anybody complains :)

	Rupesh
	Comment 3 • 18 years ago

(In reply to comment #2)
> Created an attachment (id=233941) [edit]
> patch
> 
Christian,
Above patch didn't cause any errors with xlC compiler on AIX.

	David Baron :dbaron:
	Comment 4 • 18 years ago

Comment on attachment 233941 [details] [diff] [review]
patch (checked in)

sr=dbaron.  Definitely wait a bit after landing ths before proceeding with further removal.

	Christian :Biesinger (don't email me, ping me on IRC) Reporter
	Comment 5 • 18 years ago

Comment on attachment 233941 [details] [diff] [review]
patch (checked in)

Checking in nscore.h;
/cvsroot/mozilla/xpcom/base/nscore.h,v  <--  nscore.h
new revision: 1.96; previous revision: 1.95
done

	Jeff Walden [:Waldo] Assignee
	Comment 6 • 17 years ago

I think this qualifies as waiting "a bit".  :-)  We should actually follow through on removing this bit of gratuitous Moz-ness.

Taras, this might (?) be a good, simple (?) testcase for a transformation that has to deal with macros.

	Benjamin Smedberg
	Comment 7 • 17 years ago

You don't need an automated rewriter for this, perl or grep work just fine.

	(dormant account)
	Comment 8 • 17 years ago

> Taras, this might (?) be a good, simple (?) testcase for a transformation that
> has to deal with macros.
 
Benjamin is right about using perl for this. In this case swapping out macros for casts with perl should work. A tool would only beneficial if you need some context info to do your refactoring. 

I'll write a simple tool for this if perl proves insufficient.

	Jeff Walden [:Waldo] Assignee
	Comment 9 • 17 years ago

This script works correctly on a trunk tree.  It doesn't really attempt to fix up formatting.  You'll have to make a few changes before and after running it:

Before:
- the script doesn't handle "NS_STATIC_CAST\n(" correctly, so you have to
  remove these from the tree before running the script
  - docshell/base/nsDocShell.cpp has a few of these
  - extensions/wallet/src/singsign.cpp has one

After:
- the macro definitions in nscore.h get munged, too; should they be removed,
  or should I leave them and in a separate patch make use of them cause
  warnings?
- modules/libjar has its own NS_STATIC_CAST macro definition which must
  be unmunged

The full-tree diff after I did this was 3MB, touched 967 files, and took 45 minutes to generate.  Committing it would probably require closing the tree, because the tinderboxen would have a ton of mid-commit errors.  Clearly this will require some sort of directory-by-directory commit over a reasonable length of time.  :-)

	Jeff Walden [:Waldo] Assignee
	Comment 10 • 17 years ago

Comment on attachment 270453 [details]
Script which converts the tree correctly, modulo ~3 changes

By the way, I'm not all that certain grep would be up to the task here, given nested paren/commas and nested macro uses have to be fixed up correctly.  perl could do it, but I think it'd basically have to duplicate what I did here, which is what I was trying to avoid.

	Christian :Biesinger (don't email me, ping me on IRC) Reporter
	Comment 11 • 17 years ago

> Committing it would probably require closing the tree,
> because the tinderboxen would have a ton of mid-commit errors.

You don't have to check in the removal of the macro definitions together with the rest.

	Benjamin Smedberg
	Comment 12 • 17 years ago

Comment on attachment 270453 [details]
Script which converts the tree correctly, modulo ~3 changes

Please keep the NS_*_CAST macro definitions around for a while even after you convert the tree over.

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 17 years ago

I assume the script doesn't really fix up whitespace to give correct indents post-run?

	Jeff Walden [:Waldo] Assignee
	Comment 14 • 17 years ago

(In reply to comment #13)
> I assume the script doesn't really fix up whitespace to give correct indents
> post-run?

(In reply to comment #9)
> It doesn't really attempt to fix up formatting.

It does the right thing if the cast is all on one line.  If it's not, it doesn't.  Looking at some of the casts, I'm not even entirely certain what the right thing is, given that indentation's sometimes whacked for over-long lines (e.g. "[lots of indent]foo = NS_STATIC_CAST(type*,\nexpr);", where expr is just barely long enough to necessitate the break).  I considered trying to handle it, but I'm not 100% sure it's worth the effort.  If you really cared (or if I decide to revisit this some evening this week), I might be able to make something slightly less stupid.

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 17 years ago

I moderately care; dealing with the existing broken indents from things like s/GetPresContext/PresContext/ has been pretty annoying...  It leads to future patches having a lot more changes in them, complicating review.

	Jeff Walden [:Waldo] Assignee
	Comment 16 • 17 years ago

Suppose I do get some vague amount of fixup working...what alignment would you want for the following?  I don't even know what the "right" fixup would be for these in terms of alignment; there's no obvious alignment for the <> and () portions:

     NS_STATIC_CAST(nsIFoo*,
                    nsFooManager::GlobalFoo());

     NS_CONST_CAST(nsIFoo*,
                   NS_STATIC_CAST(nsIFoo*,
                                  item->mFooObject));

	Jeff Walden [:Waldo] Assignee
	Comment 17 • 17 years ago

I take it back; this wasn't quite as bad as I thought.  I don't see anything noticeably different in the normal case with this patch, which I've used on a clean tree and verified that the resulting build works correctly (still need the post-convert changes, but the pre-convert ones are in the tree now); in the line-break case, I get the following results (before/after for each):

 nsIFoo* foo = NS_STATIC_CAST(nsIFoo*,
                              NS_STATIC_CAST(nsIBar*,
                                             this->mBar));
 nsIFoo* foo = static_cast<nsIFoo*>
                          (static_cast<nsIBar*>
                                      (this->mBar));

This keeps opening paren/bracket aligned...


 nsIFoo* foo = NS_STATIC_CAST(nsIFoo*,
                              NS_STATIC_CAST(nsIBar*,
                                             NS_STATIC_CAST(nsIQuux*,
                                                            voidPtr)));
 nsIFoo* foo = static_cast<nsIFoo*>
                          (static_cast<nsIBar*>
                                      (static_cast<nsIQuux*>
                                                  (voidPtr)));

...to more than one level, if that even matters in practice.


 nsIFoo* foo = NS_STATIC_CAST(Bar*, NS_STATIC_CAST(void*,
                                                   objectPtr));
 nsIFoo* foo = static_cast<Bar*>(static_cast<void*>
                                      (objectPtr));

It fails on a broken cast inside a single-line, but a search for lines with two NS_.*_CASTs in them produces only 10ish that match this pattern (in most such cases both are single-line or separate casts); I can fix them up manually without much effort (less than exposing that info in the script would require).

This patch is a bit more magical than the previous one, however; I didn't spend much time reasoning through my propagation of indentation -- some thinking, some trial-and-error based on tests.  That said, it seems to work okay, and skimming some of the changes in layout doesn't show any that aren't correct whitespace-wise.

bz, does this sound better to you?

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 17 years ago

That sounds great, thanks!  As long as it gets the common case right, just a few "broken" indents are not something to worry about, by the way.

	Jeff Walden [:Waldo] Assignee
	Comment 19 • 17 years ago

For anyone else looking to use this outside the Moz tree, this version (when finished converting) doesn't store backups of everything, and it preserves timestamps on unmodified files.

I converted a Firefox checkout a few hours ago.  Camino, SeaMonkey, Thunderbird, etc. and anything else not in a Firefox checkout still have to be converted, but I'll get to them sometime soonish.

	neil@parkwaycc.co.uk
	Comment 20 • 17 years ago

This fixes the alignment of the trailing \s used in macros, because xxx_cast<foo*>(bar) is three characters shorter than NS_XXX_CAST(foo*, bar)

	neil@parkwaycc.co.uk
	Comment 21 • 17 years ago

(In reply to comment #20)
>Created an attachment (id=271808)
Forget the nsINode changes, they've bitrotted since I created the patch.

	Christian :Biesinger (don't email me, ping me on IRC) Reporter
	Comment 22 • 17 years ago

-> waldo, who is doing the remaining work here

	Jeff Walden [:Waldo] Assignee
	Comment 23 • 17 years ago

All instances of the macros have been removed, and the definitions themselves are now gone!  FIXED.