bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th from 16:00 until 20:00 UTC.

make NS_*_CAST always use C++ casts

RESOLVED FIXED in mozilla1.9alpha8



12 years ago
10 years ago


(Reporter: Biesinger, Assigned: Waldo)


Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)



(6 attachments)

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:
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
Created attachment 233941 [details] [diff] [review]
patch (checked in)
Attachment #233941 - Flags: superreview?(dbaron)
Attachment #233941 - Flags: review?(benjamin)


12 years ago
Attachment #233941 - Flags: review?(benjamin) → review+

Comment 3

12 years ago
(In reply to comment #2)
> Created an attachment (id=233941) [edit]
> patch
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
Attachment #233941 - Attachment description: patch → patch (checked in)

Comment 6

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

Comment 7

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

Comment 8

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

Comment 9

11 years ago
Created attachment 270453 [details]
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:

- 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

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


11 years ago
Attachment #270453 - Attachment mime type: application/octet-stream → text/x-python

Comment 10

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

11 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.
Attachment #270453 - Flags: review?(benjamin) → review+
I assume the script doesn't really fix up whitespace to give correct indents post-run?

Comment 14

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

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



Comment 17

11 years ago
Created attachment 270709 [details]
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*,
 nsIFoo* foo = static_cast<nsIFoo*>

This keeps opening paren/bracket aligned...

 nsIFoo* foo = NS_STATIC_CAST(nsIFoo*,
 nsIFoo* foo = static_cast<nsIFoo*>

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

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

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.

Comment 19

11 years ago
Created attachment 271413 [details]
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

11 years ago
Created attachment 271808 [details] [diff] [review]
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

11 years ago
(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

Comment 23

11 years ago
All instances of the macros have been removed, and the definitions themselves are now gone!  FIXED.
Last Resolved: 11 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.