348748 - make NS_*_CAST always use C++ casts

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

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 :)

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: my test program

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch (checked in)

Comment 3

[Rupesh]

(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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

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

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 6

[Jeff Walden [:Waldo]]

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.

Comment 7

[Benjamin Smedberg]

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

Comment 8

[(dormant account)]

> 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.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Script which converts the tree correctly, modulo ~3 changes

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.  :-)

Comment 10

[Jeff Walden [:Waldo]]

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.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

> 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.

Comment 12

[Benjamin Smedberg]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Jeff Walden [:Waldo]]

(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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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.

Comment 16

[Jeff Walden [:Waldo]]

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));

Comment 17

[Jeff Walden [:Waldo]]

Attachment: Convert, with alignment preservation

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?

Comment 18

[Boris Zbarsky [:bzbarsky]]

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.

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Slight tweak to not bump timestamps on unmodified files, dele old versions when done

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.

Comment 20

[[e-mail]]

Attachment: Fix trailing backslash alignment

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)

Comment 21

[[e-mail]]

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

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

-> waldo, who is doing the remaining work here

Comment 23

[Jeff Walden [:Waldo]]

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