Closed Bug 348748 Opened 13 years ago Closed 12 years ago

make NS_*_CAST always use C++ casts

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: Biesinger, Assigned: Waldo)

References

Details

Attachments

(6 files)

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 :)
OS: Linux → All
Hardware: PC → All
Attachment #233941 - Flags: superreview?(dbaron)
Attachment #233941 - Flags: review?(benjamin)
Attachment #233941 - Flags: review?(benjamin) → review+
(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 on attachment 233941 [details] [diff] [review]
patch (checked in)

sr=dbaron.  Definitely wait a bit after landing ths before proceeding with further removal.
Attachment #233941 - Flags: superreview?(dbaron) → superreview+
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
Attachment #233941 - Attachment description: patch → patch (checked in)
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.
You don't need an automated rewriter for this, perl or grep work just fine.
> 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.
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.  :-)
Attachment #270453 - Flags: review?(benjamin)
Attachment #270453 - Attachment mime type: application/octet-stream → text/x-python
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.
Attachment #270453 - Attachment mime type: text/x-python → text/plain
> 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 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.
Attachment #270453 - Flags: review?(benjamin) → review+
I assume the script doesn't really fix up whitespace to give correct indents post-run?
(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.
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.
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));
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?
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.
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.
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)
(In reply to comment #20)
>Created an attachment (id=271808)
Forget the nsINode changes, they've bitrotted since I created the patch.
-> waldo, who is doing the remaining work here
Assignee: cbiesinger → jwalden+bmo
All instances of the macros have been removed, and the definitions themselves are now gone!  FIXED.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M8
You need to log in before you can comment on or make changes to this bug.