Closed Bug 784739 Opened 12 years ago Closed 10 years ago

Switch from NULL to nullptr

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: poiru)

References

Details

(Whiteboard: [mentor=ehsan][lang=c++][qa-])

Attachments

(79 files, 58 obsolete files)

26.46 KB, patch
ehsan.akhgari
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
27.12 KB, patch
ehsan.akhgari
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
27.14 KB, patch
ehsan.akhgari
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
20.60 KB, patch
ehsan.akhgari
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
16.62 KB, patch
ehsan.akhgari
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
13.42 KB, patch
ehsan.akhgari
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
23.36 KB, patch
ehsan.akhgari
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
39.74 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
104.10 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
53.30 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
37.11 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
86.23 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
25.12 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
137.86 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
150.58 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
38.42 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
87.99 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
77.60 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
75.67 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
63.67 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
81.88 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
67.09 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
77.66 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
94.68 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
86.84 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
84.84 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
79.72 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
86.66 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
90.86 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
86.26 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
78.79 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
76.10 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
76.19 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
79.84 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
76.23 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
77.34 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
76.56 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
80.49 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
83.69 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
84.95 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
102.41 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
48.13 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
23.79 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
78.45 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
78.66 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
70.07 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
68.63 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
93.62 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
79.65 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
54.61 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
68.23 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
77.77 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
85.38 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
78.84 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
80.46 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
29.04 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
57.52 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
66.64 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
85.54 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
44.08 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
16.06 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
42.41 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
29.04 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
53.37 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
49.32 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
8.00 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
28.13 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
30.95 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
41.02 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
35.43 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
50.97 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
42.51 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
15.77 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
16.92 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
30.86 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
97.32 KB, patch
ehsan.akhgari
: review+
ehugg
: feedback+
Details | Diff | Splinter Review
33.19 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
18.34 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
3.74 KB, patch
Details | Diff | Splinter Review
This is the follow-up to bug 626472.
Josh, do you know any contributors who might wanna take this over?  This is not necessarily a good first bug.
Depends on: 783206
What must we do for resolve this "bug" please ? Just replace all NULL by nullptr ?
And, must we have Mac for this "bug" ?
(In reply to Kévin Plestan from comment #2)
> What must we do for resolve this "bug" please ? Just replace all NULL by
> nullptr ?

Yes.

(In reply to Kévin Plestan from comment #3)
> And, must we have Mac for this "bug" ?

No.
Ok Thank you, I will try to do this
Excuse me, must I search file by file "NULL" and replace it by "nullptr" or there is something which can help me ? Something like an IDE ? Thank you for your help.
Must I replace all NULL I see by nullptr or must I just replace the NULL which are used for pointer ? Sorry for silly questions but I do not know all of the programming's world.
Thanks Kévin for your interest in working on this!

I think the best way to replace NULL with nullptr across all of our code is to use a script similar to the one used in bug 690892 <https://bug690892.bugzilla.mozilla.org/attachment.cgi?id=567444>.  You can just replace the sed rules there with ones that replace NULL with nullptr, something like:

's/\bNULL    /nullptr /g'
's/\bNULL\b/nullptr/g'

The spacing trick is used to make sure that we avoid messing up the indentation as much as we can.

Please let me know if you need more help.
Must I modified .idl files ?

Thank you.
(In reply to comment #9)
> Must I modified .idl files ?

No.
Thank you very much
@Kevin: I'd love to take this over if you're no longer working on this. :)
I'm at the end of my work. I'm trying to compile my mozilla's version. I'll informe you if it's working ;) .

P.S : correct me if I do english errors because I'm not english (but french. Besides, a french who's learning english ^^ ) Thank You !
(In reply to comment #13)
> I'm at the end of my work. I'm trying to compile my mozilla's version. I'll
> informe you if it's working ;) .

Great to hear that!

> P.S : correct me if I do english errors because I'm not english (but french.
> Besides, a french who's learning english ^^ ) Thank You !

No worries.  A lot of us are not native English speakers either (including myself.  :-)
@Kevin: Still working on this? I'd love to fix this bug if you're no longer interested. :)
I've tried changing NULL to nullptr in all cpp, cc, mm and h files and, because I changed source/mfbt/Assertions.h which is used by memory/mozjemalloc/jemalloc.c so this gives me an error. Should I avoid changing the h files?
(In reply to Cristian Ojog from comment #16)
> I've tried changing NULL to nullptr in all cpp, cc, mm and h files and,
> because I changed source/mfbt/Assertions.h which is used by
> memory/mozjemalloc/jemalloc.c so this gives me an error. Should I avoid
> changing the h files?

The majority of h files can use nullptr; I think avoiding Assertions.h will probably be sufficient.
Hi I tried this bug with command 
find ./ ! -path ./.hg/\* ! -name Assertions.h -type f \( -iname \*.h -o -iname \*.cpp -o -iname *.mm -o -iname *.cc \) | xargs -n 1 sed -i -e 's/\bNULL /nullptr /g' -e 's/\bNULL\b/nullptr/g'

but the build is showing
error: ‘nullptr’ undeclared (first use in this function)
do i have to include <mfbt/nullptr.h> to files too ?
and which directories do i have to exclude other than .hg ?
Assignee: nobody → mohit.www
Status: NEW → ASSIGNED
(In reply to comment #18)
> Hi I tried this bug with command 
> find ./ ! -path ./.hg/\* ! -name Assertions.h -type f \( -iname \*.h -o -iname
> \*.cpp -o -iname *.mm -o -iname *.cc \) | xargs -n 1 sed -i -e 's/\bNULL
> /nullptr /g' -e 's/\bNULL\b/nullptr/g'
> 
> but the build is showing
> error: ânullptrâ undeclared (first use in this function)
> do i have to include <mfbt/nullptr.h> to files too ?
> and which directories do i have to exclude other than .hg ?

Which file is this error coming from?
/home/mohit/mozilla-central/memory/mozjemalloc/jemalloc.c: In function ‘extent_tree_szad_first’:
/home/mohit/mozilla-central/memory/mozjemalloc/jemalloc.c:2307:1: error: ‘nullptr’ undeclared (first use in this function)

Same error in functions
extent_tree_szad_[first/last/next/prev/nsearch/search/psearch]
extent_tree_ad_[first/last/next/prev/nsearch/search/psearch]
arena_chunk_tree_dirty_[first/last/next/prev/nsearch/search/psearch]
arena_avail_tree_[first/last/next/prev/nsearch/search/psearch]
You probably should leave memory/mozjemalloc and memory/jemalloc alone.
(In reply to :Ms2ger from comment #21)
> You probably should leave memory/mozjemalloc and memory/jemalloc alone.

Now these error are present
/home/mohit/mozilla-central/nsprpub/pr/src/io/prfdcach.c: In function ‘_PR_Getfd’:
 /home/mohit/mozilla-central/nsprpub/pr/src/io/prfdcach.c:113:33: error: ‘nullptr’ undeclared (first use in this function)
 /home/mohit/mozilla-central/nsprpub/pr/src/io/prfdcach.c:113:33: note: each undeclared identifier is reported only once for each function it appears in
 /home/mohit/mozilla-central/nsprpub/pr/src/io/prfdcach.c: In function ‘_PR_CleanupFdCache’:
  /home/mohit/mozilla-central/nsprpub/pr/src/io/prfdcach.c:274:9: error: ‘nullptr’ undeclared (first use in this function)

I modified the command to :
find ./  ! -path ./.hg/\* ! -path ./memory/mozjemalloc/\* ! -path ./memory/jemalloc/\* ! -name Assertions.h -type f \( -iname \*.h -o -iname \*.cpp -o -iname *.mm -o -iname *.cc \) | xargs -n 1 sed -i -e 's/\bNULL /nullptr /g' -e 's/\bNULL\b/nullptr/g'
Please exclude all .c files.  Thanks!
Attachment #701145 - Flags: review?(Ms2ger)
Attached file command used to create the patch (obsolete) —
I excluded mozilla-central/toolkit/* because it was showing an "nullptr" not defined error in files
mozilla-central/toolkit/crashreporter/google-breakpad/src/common/*.cc

and the patch was too large so i had to upload the compressed version.
Attached file script (obsolete) —
Attachment #701146 - Attachment is obsolete: true
Attachment #701145 - Attachment is obsolete: true
Attachment #701145 - Flags: review?(Ms2ger)
Attachment #701300 - Flags: feedback?(Ms2ger)
Thanks for the patch; I'll try to take a look tomorrow.
Comment on attachment 701300 [details] [diff] [review]
NULL changed to nullptr in .cc and .cpp files

Sorry it took a little longer than I expected. I skimmed over the patch, and it generally looks good. There's a few more things you should leave alone, though:


You shouldn't touch the strings in dom/indexedDB/OpenDatabaseHelper.cpp.

Please fix the indentation of the backslashes of COPY_STRING in dom/workers/Navigator.cpp and dom/workers/WorkerScope.cpp (they should all line up).

extensions/spellcheck/hunspell/src/ is upstream code; you should leave that alone.

Same for gfx/angle/, gfx/cairo/cairo/src/, gfx/graphite2/, gfx/harfbuzz/src/, gfx/ots/src/, gfx/skia/src/ and gfx/ycbcr/.

Same for media/libsoundtouch/, mfbt/double-conversion/ and other-licenses/.


Thanks for taking this on!
Attachment #701300 - Flags: feedback?(Ms2ger) → feedback+
Attached patch bug 784739 fix (obsolete) — Splinter Review
Attachment #701152 - Attachment is obsolete: true
Attachment #701300 - Attachment is obsolete: true
Attachment #706563 - Flags: review?(Ms2ger)
Attached file command (obsolete) —
Attachment #706563 - Flags: review?(ehsan)
Comment on attachment 706564 [details]
command

>find ./ \
>! -path ./.hg/\* \
>! -path ./extensions/spellcheck/hunspell/src\* \
>! -path ./toolkit\* \

Why are you excluding toolkit?

>! -path \*netwerk/sctp\* \
>! -path \*xpcom/typelib/xpt/\* \
>! -path ./memory/mozjemalloc/\* \
>! -path ./js\* ! -path \*nsprpub\* \
>! -path ./memory/jemalloc/\* \
>! -path ./gfx/angle/\* \
>! -path ./gfx/cairo/cairo/src/\* \
>! -path ./gfx/graphite2/\* \
>! -path ./gfx/harfbuzz/src/\* \
>! -path ./gfx/ots/src/\* \
>! -path ./gfx/skia/src/\* \
>! -path ./gfx/ycbcr/\* \
>! -path ./media/libsoundtouch/\* \
>! -path ./mfbt/double-conversion/\* \
>! -path ./other-licenses/\* \
>! -name Assertions.h \
>! -name OpenDatabaseHelper.cpp \
>-type f \( -iname \*.cpp -o -iname *.cc \) | xargs -n 1 sed -i -e 's/\bNULL /nullptr /g' -e 's/\bNULL\b/nullptr/g'

This will mess up indentations.  You want something like: 's/\bNULL    /nullptr /g'.

Also, have you run this through the try server?  If yes, please post a TBPL link.  Thanks!
Attachment #706563 - Flags: review?(ehsan)
Randell, what parts of the code in media/webrtc is owned by us?
Well, these are not our code:

media/webrtc/trunk
media/webrtc/signaling, except for:
  media/webrtc/signaling/src/test
  media/webrtc/signaling/src/media-conduit
  media/webrtc/signaling/src/mediapipeline
  media/webrtc/signaling/src/peerconnection

The media/webrtc/signaling code other than ours has largely been taken over by us though, so we can discuss making reasonably portable changes to it for stuff like this.  Ethan Hugg of Cisco (ehugg on #media) and I would be the people to talk to.

In other news, media/mtransport/third_party are all imported, as is netwerk/srtp/src and netwerk/sctp/src
Thanks, Randell!

So, Mohit, to keep things simper here, I'd suggest you exclude media/webrtc, media/mtransport/third_party, netwerk/srtp/src and netwerk/sctp/src for now.  Thanks!
Attached patch NULL changed to nullptr (obsolete) — Splinter Review
Attachment #706563 - Attachment is obsolete: true
Attachment #706564 - Attachment is obsolete: true
Attachment #706563 - Flags: review?(Ms2ger)
Attachment #708398 - Flags: review?(ehsan)
Attached file Updated command (obsolete) —
As you can see in the link in comment 39, this patch doesn't build on any platform.
Attachment #708398 - Flags: review?(ehsan)
Hi all, 

as there was no activity in this ticket since end of January, I would like to take it over. Is it ok to re-assign it to me?
Assignee: mohit.www → matekm
This is first version of the patch that changes occurences of NULL to nullptr
Attachment #727864 - Flags: review?(matekm)
Would like to run this patch through try server, but I need level 1 access and for that I need a voucher. Please, vote on https://bugzilla.mozilla.org/show_bug.cgi?id=851871
New, gzipped patch. Previous one wasn't full.If there is a need I can split this patch in few smaller ones.
Attachment #727864 - Attachment is obsolete: true
Attachment #727864 - Flags: review?(matekm)
Attachment #727874 - Flags: review?(matekm)
Instead of gzipping it, I advise breaking it into one patch per top-level directory (or some such)
Attachment #727874 - Attachment is obsolete: true
Attachment #727874 - Flags: review?(matekm)
Attachment #729634 - Flags: review?(matekm)
Attachment #729635 - Flags: review?(matekm)
Attachment #729637 - Flags: review?(matekm)
Attachment #729640 - Flags: review?(matekm)
Attachment #729641 - Flags: review?(matekm)
Attachment #729642 - Flags: review?(matekm)
Attachment #729643 - Flags: review?(matekm)
Attachment #729644 - Flags: review?(matekm)
Attachment #729645 - Flags: review?(matekm)
Attachment #729646 - Flags: review?(matekm)
Attachment #729648 - Flags: review?(matekm)
Attachment #729649 - Flags: review?(matekm)
Attachment #729650 - Flags: review?(matekm)
Attachment #729651 - Flags: review?(matekm)
Attachment #729653 - Flags: review?(matekm)
Attachment #729640 - Attachment is obsolete: true
Attachment #729640 - Attachment is patch: false
Attachment #729640 - Flags: review?(matekm)
Why are these all assigned for yourself to review?
Attachment #729634 - Flags: review?(matekm) → review?(ehsan)
Attachment #729635 - Flags: review?(matekm) → review?(ehsan)
Attachment #729637 - Flags: review?(matekm) → review?(ehsan)
Attachment #729641 - Flags: review?(matekm) → review?(ehsan)
Attachment #729642 - Flags: review?(matekm) → review?(ehsan)
Attachment #729643 - Flags: review?(matekm) → review?(ehsan)
Attachment #729644 - Flags: review?(matekm) → review?(ehsan)
Attachment #729645 - Flags: review?(matekm) → review?(ehsan)
Attachment #729646 - Flags: review?(matekm) → review?(ehsan)
Attachment #729648 - Flags: review?(matekm) → review?(ehsan)
Attachment #729649 - Flags: review?(matekm) → review?(ehsan)
Attachment #729650 - Flags: review?(matekm) → review?(ehsan)
Attachment #729651 - Flags: review?(matekm) → review?(ehsan)
Attachment #729653 - Flags: review?(matekm) → review?(ehsan)
I don't expect to get to these reviews before next week...  Sorry.
Comment on attachment 729649 [details] [diff] [review]
mozilla-central/editor subdir NULL -> nullptr switch

Review of attachment 729649 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/html/nsEditorTxnLog.cpp
@@ +307,4 @@
>    if (txn) {
>      txn->GetTxnDescription(str);
>      if (str.IsEmpty())
> +      str.AssignLiteral("<nullptr>");

Hmm, this is bad, but it doesn't matter in this case since this code is unused...
Attachment #729649 - Flags: review?(ehsan) → review+
Whiteboard: [mentor=ehsan][lang=c++] → [mentor=ehsan][lang=c++][leave open]
Comment on attachment 729649 [details] [diff] [review]
mozilla-central/editor subdir NULL -> nullptr switch

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8abe42df0dc
Attachment #729649 - Flags: checkin+
Attachment #729642 - Flags: review?(ehsan) → review+
Attachment #729641 - Flags: review?(ehsan) → review+
Comment on attachment 729643 [details] [diff] [review]
mozilla-central/config subdir NULL -> nullptr switch

Review of attachment 729643 [details] [diff] [review]:
-----------------------------------------------------------------

This file is not used, and it should probably be removed.  Can you please file a bug on that?  Thanks!
Attachment #729643 - Flags: review?(ehsan) → review-
Attachment #729635 - Flags: review?(ehsan) → review+
Comment on attachment 729642 [details] [diff] [review]
mozilla-central/chrome subdir NULL -> nullptr switch

https://hg.mozilla.org/integration/mozilla-inbound/rev/275cff0aef99
Attachment #729642 - Flags: checkin+
Comment on attachment 729641 [details] [diff] [review]
mozilla-central/caps subdir NULL -> nullptr switch

https://hg.mozilla.org/integration/mozilla-inbound/rev/832de954da6a
Attachment #729641 - Flags: checkin+
Comment on attachment 729635 [details] [diff] [review]
mozilla-central/b2g subdir NULL -> nullptr switch

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ab6aaf3ed13
Attachment #729635 - Flags: checkin+
Comment on attachment 729645 [details] [diff] [review]
mozilla-central/dbm subdir NULL -> nullptr switch

Review of attachment 729645 [details] [diff] [review]:
-----------------------------------------------------------------

We do not own this code.  Sorry, I should have mentioned it earlier.
Attachment #729645 - Flags: review?(ehsan) → review-
Attachment #729646 - Flags: review?(ehsan) → review+
Comment on attachment 729650 [details] [diff] [review]
mozilla-central/embedding subdir NULL -> nullptr switch

Review of attachment 729650 [details] [diff] [review]:
-----------------------------------------------------------------

::: embedding/components/find/src/nsFind.cpp
@@ +466,4 @@
>  {
>    if (!aNode)
>    {
> +    printf(">>>> Node: nullptr\n");

This is not needed, but I'll remove it myself.
Attachment #729650 - Flags: review?(ehsan) → review+
Comment on attachment 729646 [details] [diff] [review]
mozilla-central/docshell subdir NULL -> nullptr switch

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b2c0b85aea3
Attachment #729646 - Flags: checkin+
Comment on attachment 729650 [details] [diff] [review]
mozilla-central/embedding subdir NULL -> nullptr switch

https://hg.mozilla.org/integration/mozilla-inbound/rev/16af89e4b032
Attachment #729650 - Flags: checkin+
Comment on attachment 729634 [details] [diff] [review]
mozilla-central/accessible subdir NULL -> nullptr switch

Review of attachment 729634 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/integration/mozilla-inbound/rev/6120f41441ff
Attachment #729634 - Flags: review?(ehsan)
Attachment #729634 - Flags: review+
Attachment #729634 - Flags: checkin+
Comment on attachment 729637 [details] [diff] [review]
mozilla-central/browser subdir NULL -> nullptr switch

Review of attachment 729637 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/integration/mozilla-inbound/rev/e56f91d780df
Attachment #729637 - Flags: review?(ehsan)
Attachment #729637 - Flags: review+
Attachment #729637 - Flags: checkin+
Comment on attachment 729651 [details] [diff] [review]
mozilla-central/extensions subdir NULL -> nullptr switch

Review of attachment 729651 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d063f04ee2
Attachment #729651 - Flags: review?(ehsan)
Attachment #729651 - Flags: review+
Attachment #729651 - Flags: checkin+
Comment on attachment 729644 [details] [diff] [review]
mozilla-central/content subdir NULL -> nullptr switch

Review of attachment 729644 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/smil/nsSMILAnimationFunction.cpp
@@ +421,5 @@
>      }
>  
>      if (NS_SUCCEEDED(rv)) {
> +      NS_ABORT_IF_FALSE(from, "nullptr from-value during interpolation");
> +      NS_ABORT_IF_FALSE(to, "nullptr to-value during interpolation");

These are not needed, will fix myself.

::: content/smil/nsSMILNullType.cpp
@@ +33,4 @@
>  nsSMILNullType::Add(nsSMILValue& aDest, const nsSMILValue& aValueToAdd,
>                      uint32_t aCount) const
>  {
> +  NS_NOTREACHED("Adding nullptr type");

Ditto.

::: content/smil/nsSMILTimedElement.cpp
@@ +267,4 @@
>  void
>  nsSMILTimedElement::SetAnimationElement(SVGAnimationElement* aElement)
>  {
> +  NS_ABORT_IF_FALSE(aElement, "nullptr owner element");

Ditto.

::: content/svg/content/src/SVGContentUtils.cpp
@@ +70,4 @@
>  float
>  SVGContentUtils::GetFontSize(nsIFrame *aFrame)
>  {
> +  NS_ABORT_IF_FALSE(aFrame, "nullptr frame in GetFontSize");

And here.

::: content/svg/content/src/nsSVGAttrTearoffTable.h
@@ +55,4 @@
>  #endif
>      mTable.Get(aSimple, &tearoff);
>    NS_ABORT_IF_FALSE(!found || tearoff,
> +      "nullptr pointer stored in attribute tear-off map");

And here.
Attachment #729644 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #74)
> Comment on attachment 729637 [details] [diff] [review]
> mozilla-central/browser subdir NULL -> nullptr switch
> 
> Review of attachment 729637 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e56f91d780df

Landed a follow-up: https://hg.mozilla.org/integration/mozilla-inbound/rev/06ae36377cfa
Comment on attachment 729637 [details] [diff] [review]
mozilla-central/browser subdir NULL -> nullptr switch

This broke the build again this time somewhere else on Windows, so I backed it out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/84588077a188

Build failure:

https://tbpl.mozilla.org/php/getParsedLog.php?id=21369991&tree=Mozilla-Inbound
Attachment #729637 - Flags: checkin+ → checkin-
Comment on attachment 729644 [details] [diff] [review]
mozilla-central/content subdir NULL -> nullptr switch

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad1a3999aa2e
Attachment #729644 - Flags: checkin+
Comment on attachment 729648 [details] [diff] [review]
mozilla-central/dom subdir NULL -> nullptr switch

Review of attachment 729648 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/base/android/android_npapi.h
@@ +104,4 @@
>  
>  /** queries for a specific ANPInterface.
>  
> +    Maybe called with nullptr for the NPP instance

Please revert the changes to this file.

::: dom/plugins/base/npapi.h
@@ +154,4 @@
>    void*    notifyData;
>    const    char* headers; /* Response headers from host.
>                             * Exists only for >= NPVERS_HAS_RESPONSE_HEADERS.
> +                           * Used for HTTP only; nullptr for non-HTTP.

And this file.

::: dom/plugins/base/npruntime.h
@@ +137,4 @@
>  #define VOID_TO_NPVARIANT(_v)                                                 \
>  NP_BEGIN_MACRO                                                                \
>      (_v).type = NPVariantType_Void;                                           \
> +    (_v).value.objectValue = nullptr;                                            \

And this file.

::: dom/system/gonk/android_audio/EffectApi.h
@@ +78,4 @@
>      uint8_t node[6];
>  } effect_uuid_t;
>  
> +// nullptr UUID definition (matches SL_IID_NULL_)

And this file.
Attachment #729648 - Flags: review?(ehsan) → review-
Comment on attachment 729653 [details] [diff] [review]
mozilla-central/gfx subdir NULL -> nullptr switch

Review of attachment 729653 [details] [diff] [review]:
-----------------------------------------------------------------

Please revert the changes to gfx/skia, and separate out the gfx/2d changes into another patch.  Thanks!
Attachment #729653 - Flags: review?(ehsan) → review-
Sorry for that little mess with changes. Next time I will push the patches to the try server...
Try server: https://tbpl.mozilla.org/?tree=Try&rev=62200a7dbc17
Attachment #729634 - Attachment is obsolete: true
Attachment #729635 - Attachment is obsolete: true
Attachment #729637 - Attachment is obsolete: true
Attachment #729641 - Attachment is obsolete: true
Attachment #729642 - Attachment is obsolete: true
Attachment #729643 - Attachment is obsolete: true
Attachment #729644 - Attachment is obsolete: true
Attachment #729645 - Attachment is obsolete: true
Attachment #729646 - Attachment is obsolete: true
Attachment #729648 - Attachment is obsolete: true
Attachment #729649 - Attachment is obsolete: true
Attachment #729650 - Attachment is obsolete: true
Attachment #729651 - Attachment is obsolete: true
Attachment #729653 - Attachment is obsolete: true
Attachment #734213 - Flags: review?(ehsan)
For the dom/ changes, you'll need to revert your changes to dom/indexedDB/OpenDatabaseHelper.cpp.
Hi, this is probably a very noobish question, but what would happen if I made a #define somewhere and changed NULL to nullptr? To prevent mixing C++ and C we could do this in an appropriate header file or add a level of abstraction. I'd like to work on this way of doing it and close this bug.
Last patch don't include dom changes, only browser/
(In reply to tvssarma.omega9 from comment #87)
> Hi, this is probably a very noobish question, but what would happen if I
> made a #define somewhere and changed NULL to nullptr? To prevent mixing C++
> and C we could do this in an appropriate header file or add a level of
> abstraction.

The point of this bug is to use nullptr in our source code instead of NULL for consistency.

> I'd like to work on this way of doing it and close this bug.

I believe matekm is already working on the rest of the changes needed here...
Comment on attachment 734213 [details] [diff] [review]
mozilla-central/browser subdir NULL -> nullptr switch

This patch is weird..  It seems like you've concatenated a few patch files here?  Can you please upload a version of the patch which includes all of the changes?  If you have several mq patches in your queue, you can use hg diff -r to get the difference between two changesets.
Attachment #734213 - Flags: review?(ehsan)
Could you confirm that you're still working on this?
Flags: needinfo?(matekm)
Assigning myself since matekm does not seem to be working on this any longer.
Assignee: matekm → birunthan
Flags: needinfo?(matekm)
OS: Mac OS X → All
Hardware: x86 → All
Attachment #708398 - Attachment is obsolete: true
Attachment #708399 - Attachment is obsolete: true
Attachment #734213 - Attachment is obsolete: true
Attachment #775349 - Flags: review?(ehsan)
Attachment #775350 - Flags: review?(ehsan)
Attachment #775351 - Flags: review?(ehsan)
Attachment #775353 - Flags: review?(ehsan)
Attachment #775356 - Flags: review?(ehsan)
Attachment #775357 - Flags: review?(ehsan)
Attachment #775358 - Flags: review?(ehsan)
Attachment #775359 - Flags: review?(ehsan)
Attachment #775361 - Flags: review?(ehsan)
Attachment #775362 - Flags: review?(ehsan)
Attachment #775363 - Flags: review?(ehsan)
Attachment #775364 - Flags: review?(ehsan)
Ehsan: By the way, if it is easier for you to review a few large patches (compared to many smaller patches), please let me know and I'll do so in the future.
Attachment #775349 - Flags: review?(ehsan) → review+
Comment on attachment 775350 [details] [diff] [review]
Switch from NULL to nullptr in gfx/2d/

Review of attachment 775350 [details] [diff] [review]:
-----------------------------------------------------------------

Redirecting request to Bas.
Attachment #775350 - Flags: review?(ehsan) → review?(bas)
Attachment #775350 - Flags: review?(bas) → review+
Attachment #775351 - Flags: review?(ehsan) → review+
Attachment #775353 - Flags: review?(ehsan) → review+
Attachment #775356 - Flags: review?(ehsan) → review+
Attachment #775357 - Flags: review?(ehsan) → review+
Attachment #775364 - Flags: review?(ehsan) → review+
Attachment #775358 - Flags: review?(ehsan) → review+
Attachment #775359 - Flags: review?(ehsan)
Attachment #775359 - Flags: review?(bas)
Attachment #775361 - Flags: review?(ehsan) → review+
Attachment #775363 - Flags: review?(ehsan) → review+
Comment on attachment 775362 [details] [diff] [review]
Switch from NULL to nullptr in gfx/layers/opengl/

Review of attachment 775362 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/LayerManagerOGL.h
@@ +15,5 @@
>  #ifdef XP_WIN
>  #include <windows.h>
>  #endif
>  
> +#define BUFFER_OFFSET(i) ((char *)nullptr + (i))

This code looks weird.  Joe, what's this all about?
Attachment #775362 - Flags: review?(ehsan) → review?(joe)
(In reply to comment #105)
> Ehsan: By the way, if it is easier for you to review a few large patches
> (compared to many smaller patches), please let me know and I'll do so in the
> future.

Smaller patches are always better!
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #107)
> > +#define BUFFER_OFFSET(i) ((char *)nullptr + (i))
> 
> This code looks weird.  Joe, what's this all about?

Its a pretty common idiom: http://www.embedded.com/design/embedded/4024941/Learn-a-new-trick-with-the-offsetof--macro
Attachment #775362 - Flags: review?(joe) → review+
Rebased patch.
Attachment #775350 - Attachment is obsolete: true
Attachment #775809 - Flags: review?(bas)
(In reply to comment #109)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #107)
> > > +#define BUFFER_OFFSET(i) ((char *)nullptr + (i))
> > 
> > This code looks weird.  Joe, what's this all about?
> 
> Its a pretty common idiom:
> http://www.embedded.com/design/embedded/4024941/Learn-a-new-trick-with-the-offsetof--macro

Gah, I read that as:

(char)nullptr + (i)

Sorry!
Bas: ping?
Comment on attachment 775809 [details] [diff] [review]
Switch from NULL to nullptr in gfx/2d/

Review of attachment 775809 [details] [diff] [review]:
-----------------------------------------------------------------

Only win here!
Attachment #775809 - Flags: review?(bas) → review+
Attachment #775359 - Flags: review?(bas) → review+
Birunthan, did you ever get a chance to test these patches on try?
Flags: needinfo?(birunthan)
Rebased patch.
Attachment #775361 - Attachment is obsolete: true
Attachment #778822 - Flags: review?(ehsan)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #114)
> Birunthan, did you ever get a chance to test these patches on try?

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=c4ea56e76b84
Flags: needinfo?(birunthan)
Attachment #778822 - Flags: review?(ehsan) → review+
Attachment #775349 - Attachment is obsolete: true
Attachment #775351 - Attachment is obsolete: true
Attachment #775353 - Attachment is obsolete: true
Attachment #775356 - Attachment is obsolete: true
Attachment #775357 - Attachment is obsolete: true
Attachment #775358 - Attachment is obsolete: true
Attachment #775359 - Attachment is obsolete: true
Attachment #775362 - Attachment is obsolete: true
Attachment #775363 - Attachment is obsolete: true
Attachment #775364 - Attachment is obsolete: true
Attachment #775809 - Attachment is obsolete: true
Attachment #778822 - Attachment is obsolete: true
Attachment #780465 - Flags: review?(ehsan)
Attachment #780466 - Flags: review?(ehsan)
Attachment #780467 - Flags: review?(ehsan)
Attachment #780468 - Flags: review?(ehsan)
Attachment #780469 - Flags: review?(ehsan)
Attachment #780470 - Flags: review?(ehsan)
Attachment #780471 - Flags: review?(ehsan)
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=f9c8d4306072
Attachment #780473 - Flags: review?(ehsan)
Updated to fix compile errors on some platforms.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=48f5570c3b3d
Attachment #780466 - Attachment is obsolete: true
Attachment #780466 - Flags: review?(ehsan)
Attachment #780491 - Flags: review?(ehsan)
Comment on attachment 780491 [details] [diff] [review]
Switch from NULL to nullptr in mfbt/

Review of attachment 780491 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/RangedPtr.h
@@ +14,5 @@
>  
>  #include "mozilla/Assertions.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/Util.h"
> +#include "mozilla/NullPtr.h"

NullPtr goes before Util
Attachment #780491 - Flags: review?(ehsan) → review?(jwalden+bmo)
(In reply to :Ms2ger from comment #128)
> NullPtr goes before Util

Fixed, thanks!
Attachment #780491 - Attachment is obsolete: true
Attachment #780491 - Flags: review?(jwalden+bmo)
Attachment #780552 - Flags: review?(jwalden+bmo)
Attachment #780552 - Attachment filename: 784739-mfbt.patch
Attachment #780552 - Attachment description: 784739-mfbt.patch → Switch from NULL to nullptr in mfbt/
Attachment #780552 - Attachment filename: 784739-mfbt.patch
Comment on attachment 780552 [details] [diff] [review]
Switch from NULL to nullptr in mfbt/

Review of attachment 780552 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with the one nit fixed.  Do you need someone to push this?  I can make that fix and push this, if you want, or you can post an appropriately revised patch and someone else can push it, if you'd prefer.

::: mfbt/tests/TestPoisonArea.cpp
@@ +195,5 @@
>    FormatMessageA(FORMAT_MESSAGE_ALLOCATE_BUFFER |
>                   FORMAT_MESSAGE_FROM_SYSTEM |
>                   FORMAT_MESSAGE_IGNORE_INSERTS,
> +                 nullptr, errcode, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
> +                 (LPSTR)&errmsg, 0, nullptr);

This header doesn't #include "mozilla/NullPtr.h".  It needs to #include that for this to be okay.  Right now, you're probably only succeeding because you're on a compiler with nullptr support.
Attachment #780552 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #130)
> ... I can make that fix and push this ...

Sure, that'd be great. Thanks!
Got it.  Queued up for the next time I have stuff to push.
Attachment #780467 - Flags: review?(ehsan) → review+
Attachment #780468 - Flags: review?(ehsan) → review+
Attachment #780469 - Flags: review?(ehsan) → review+
Attachment #780470 - Flags: review?(ehsan) → review+
Attachment #780471 - Flags: review?(ehsan) → review+
Comment on attachment 780473 [details] [diff] [review]
Switch from NULL to nullptr in storage/

Review of attachment 780473 [details] [diff] [review]:
-----------------------------------------------------------------

::: storage/src/TelemetryVFS.cpp
@@ +393,2 @@
>      // so we always define them.  Verify that we're not going to call
> +    // nullptr pointers, though.

Nit: s/nullptr pointers/nullptrs/
Attachment #780473 - Flags: review?(ehsan) → review+
Attachment #780465 - Flags: review?(ehsan) → review+
Pushed the mfbt/ changes, with minor post-review tweaks I noticed while double-checking |hg o -p|:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c10c0a6270ec

Thanks!
Something in this push made Windows checktests mad. Backed out per our IRC conversation.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbf37166d07c
I fired up a Windows build, and the jsapi-tests executable (which failed in tbpl logs, although nobody noticed it just at the time) was crashing in PRMJ_ShutdownNow() code, or something similar, clearly implicating the JS_Init changes -- not this bug.  Relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e978bb426e87
Rebased patch.
Attachment #780465 - Attachment is obsolete: true
Attachment #783670 - Flags: review?(ehsan)
Rebased patch.
Attachment #780471 - Attachment is obsolete: true
Attachment #783671 - Flags: review?(ehsan)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #133)
> Nit: s/nullptr pointers/nullptrs/

Fixed.
Attachment #780473 - Attachment is obsolete: true
Attachment #783674 - Flags: review?(ehsan)
Attachment #780552 - Attachment is obsolete: true
Attachment #783670 - Flags: review?(ehsan) → review+
Attachment #783671 - Flags: review?(ehsan) → review+
Attachment #783674 - Flags: review?(ehsan) → review+
Attachment #780467 - Attachment is obsolete: true
Attachment #780468 - Attachment is obsolete: true
Attachment #780469 - Attachment is obsolete: true
Attachment #780470 - Attachment is obsolete: true
Attachment #783670 - Attachment is obsolete: true
Attachment #783671 - Attachment is obsolete: true
Attachment #783674 - Attachment is obsolete: true
Attachment #793483 - Flags: review?(ehsan)
Attachment #793484 - Flags: review?(ehsan)
Attachment #793484 - Flags: review?(ehsan) → review+
Attachment #793483 - Flags: review?(ehsan) → review+
Hey Birunthan, did I ever mention how much your work here is appreciated?! :-)  Thanks a lot!
Keywords: checkin-needed
Attachment #793483 - Flags: checkin+
Attachment #793484 - Flags: checkin+
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #146)
> Hey Birunthan, did I ever mention how much your work here is appreciated?!
> :-)  Thanks a lot!

Glad to help.
Attachment #793483 - Attachment is obsolete: true
Attachment #793484 - Attachment is obsolete: true
Attachment #794427 - Flags: review?(ehsan)
Attachment #794428 - Flags: review?(ehsan)
Attachment #794430 - Flags: review?(ehsan)
Attachment #794431 - Flags: review?(ehsan)
Attachment #794432 - Flags: review?(ehsan)
Attachment #794427 - Flags: review?(ehsan) → review+
Attachment #794428 - Flags: review?(ehsan) → review+
Attachment #794430 - Flags: review?(ehsan) → review+
Attachment #794431 - Flags: review?(ehsan) → review+
Attachment #794432 - Flags: review?(ehsan) → review+
Keywords: checkin-needed
Comment on attachment 793483 [details] [diff] [review]
Switch from NULL to nullptr in browser/components/

These aren't obsolete just because they were already checked in.
Attachment #793483 - Attachment is obsolete: false
Attachment #793484 - Attachment is obsolete: false
Attachment #794427 - Flags: checkin+
Attachment #794428 - Flags: checkin+
Attachment #794430 - Flags: checkin+
Attachment #794431 - Flags: checkin+
Attachment #794432 - Flags: checkin+
Attachment #807195 - Flags: review?(ehsan)
I overzealously added `#include "mozilla/NullPtr.h"` to all public JS headers that I touched. Without it, B2G builds failed since the GCC version they use lacks support for nullptr. If you want, I can certainly look into removing the #include except where absolutely necessary.
Attachment #807199 - Flags: review?(ehsan)
The try link in the previous comment was wrong. The correct url is: https://tbpl.mozilla.org/?tree=Try&rev=6a81463451c3
Attachment #807195 - Attachment is obsolete: true
Attachment #807195 - Flags: review?(ehsan)
Attachment #807204 - Flags: review?(ehsan)
Attachment #807206 - Flags: review?(ehsan)
Attachment #807199 - Flags: review?(ehsan) → review+
Attachment #807201 - Flags: review?(ehsan) → review+
(In reply to Birunthan Mohanathas [:poiru] from comment #159)
> Created attachment 807199 [details] [diff] [review]
> Switch from NULL to nullptr in js/ductwork/, js/ipc/, and js/public/
> 
> I overzealously added `#include "mozilla/NullPtr.h"` to all public JS
> headers that I touched. Without it, B2G builds failed since the GCC version
> they use lacks support for nullptr. If you want, I can certainly look into
> removing the #include except where absolutely necessary.

No, that's fine!
Comment on attachment 807204 [details] [diff] [review]
Switch from NULL to nullptr in netwerk/ (1/2)

Review of attachment 807204 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/build/nsNetModule.cpp
@@ +306,5 @@
>                                                        \
>    BaseWebSocketChannel * inst;                        \
>                                                        \
> +  *aResult = nullptr;                                    \
> +  if (nullptr != aOuter) {                               \

Nit: indentation (will fix when landing)
Attachment #807204 - Flags: review?(ehsan) → review+
Attachment #807206 - Flags: review?(ehsan) → review+
Attachment #808170 - Flags: review?(ehsan)
Attachment #808172 - Flags: review?(ehsan)
Attachment #808174 - Flags: review?(ehsan)
Rebased patch.
Attachment #808170 - Attachment is obsolete: true
Attachment #808170 - Flags: review?(ehsan)
Attachment #808601 - Flags: review?(ehsan)
Rebased patch.
Attachment #808174 - Attachment is obsolete: true
Attachment #808174 - Flags: review?(ehsan)
Attachment #808602 - Flags: review?(ehsan)
Rebased patch.
Attachment #808176 - Attachment is obsolete: true
Attachment #808176 - Flags: review?(ehsan)
Attachment #808603 - Flags: review?(ehsan)
Attachment #808172 - Flags: review?(ehsan) → review+
Attachment #808173 - Flags: review?(ehsan) → review+
Attachment #808601 - Flags: review?(ehsan) → review+
Attachment #808602 - Flags: review?(ehsan) → review+
Attachment #808603 - Flags: review?(ehsan) → review+
Luke, do I have your blessing to land these JS engine patches here?
Flags: needinfo?(luke)
I think we should land the patches.  The biggest reason I see is that most (all?) of Gecko is doing it and consistency is good, but I'm also like that it reduces SHOUTCAPS.  These two things seem to outweigh the loss in terseness.

Before landing the js/src patches, though, could you please send an "I'm planning to make this change; here's why; any violent objections?" to m.d.t.js-engine.internals.  It'd be nice not to surprise everyone.
Flags: needinfo?(luke)
(In reply to Luke Wagner [:luke] from comment #176)
> Before landing the js/src patches, though, could you please send an "I'm
> planning to make this change; here's why; any violent objections?" to
> m.d.t.js-engine.internals.  It'd be nice not to surprise everyone.

Sure, done.
Thanks, but I think you missed js/src/vm/.
Also, js/src/jit.
(In reply to :Benjamin Peterson from comment #179)
> Thanks, but I think you missed js/src/vm/.

(In reply to :Benjamin Peterson from comment #180)
> Also, js/src/jit.

I have patches ready for them, but I have yet to review the patches myself. I also need to make sure they compile on B2G builds (which lack compiler support for nullptr). I'll try to do that in the evening (i.e. tomorrow morning MV time).
Attachment #811278 - Flags: review?(ehsan)
Attachment #811279 - Flags: review?(ehsan)
Attachment #811280 - Flags: review?(ehsan)
Attachment #811281 - Flags: review?(ehsan)
Attachment #811282 - Flags: review?(ehsan)
Attachment #811283 - Flags: review?(ehsan)
Attachment #811285 - Flags: review?(ehsan)
Attachment #811287 - Flags: review?(ehsan)
Attachment #811288 - Flags: review?(ehsan)
Attachment #811289 - Flags: review?(ehsan)
Attachment #811289 - Attachment is obsolete: true
Attachment #811289 - Flags: review?(ehsan)
Attachment #811290 - Flags: review?(ehsan)
Attachment #811291 - Flags: review?(ehsan)
Pushed the js/src/vm/ changes to try: https://tbpl.mozilla.org/?tree=Try&rev=d23bdcbe911d

Apologies for exhausting you! :-)
Attachment #811292 - Flags: review?(ehsan)
Attachment #811278 - Flags: review?(ehsan) → review+
Attachment #811279 - Flags: review?(ehsan) → review+
Attachment #811280 - Flags: review?(ehsan) → review+
Attachment #811281 - Flags: review?(ehsan) → review+
Attachment #811282 - Flags: review?(ehsan) → review+
Attachment #811283 - Flags: review?(ehsan) → review+
Attachment #811285 - Flags: review?(ehsan) → review+
Attachment #811286 - Flags: review?(ehsan) → review+
Attachment #811287 - Flags: review?(ehsan) → review+
Attachment #811288 - Flags: review?(ehsan) → review+
Attachment #811290 - Flags: review?(ehsan) → review+
Attachment #811291 - Flags: review?(ehsan) → review+
Attachment #811292 - Flags: review?(ehsan) → review+
Attachment #814084 - Flags: review?(ehsan)
Attachment #814086 - Flags: review?(ehsan)
Attachment #814087 - Flags: review?(ehsan)
Attachment #814088 - Flags: review?(ehsan)
Attachment #814090 - Flags: review?(ehsan)
Attachment #814091 - Flags: review?(ehsan)
Attachment #814092 - Flags: review?(ehsan)
Attachment #814093 - Flags: review?(ehsan)
Attachment #814094 - Flags: review?(ehsan)
Attachment #814084 - Flags: review?(ehsan) → review+
Attachment #814086 - Flags: review?(ehsan) → review+
Attachment #814087 - Flags: review?(ehsan) → review+
Attachment #814088 - Flags: review?(ehsan) → review+
Attachment #814090 - Flags: review?(ehsan) → review+
Attachment #814091 - Flags: review?(ehsan) → review+
Attachment #814092 - Flags: review?(ehsan) → review+
Attachment #814093 - Flags: review?(ehsan) → review+
Attachment #814094 - Flags: review?(ehsan) → review+
Attachment #814095 - Flags: review?(ehsan) → review+
Attachment #814361 - Flags: review?(ehsan)
Attachment #814363 - Flags: review?(ehsan)
Attachment #814364 - Flags: review?(ehsan)
Attachment #814365 - Flags: review?(ehsan)
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3eac0e9dc377

The B2G ICS build failed due to an unrelated reason. I can retry that if needed.
Attachment #814368 - Flags: review?(ehsan)
Attachment #814361 - Flags: review?(ehsan) → review+
Attachment #814363 - Flags: review?(ehsan) → review+
Attachment #814364 - Flags: review?(ehsan) → review+
Attachment #814365 - Flags: review?(ehsan) → review+
Attachment #814366 - Flags: review?(ehsan) → review+
Attachment #814368 - Flags: review?(ehsan) → review+
Attachment #814367 - Flags: review?(ehsan) → review+
Attachment #815461 - Flags: review?(ehsan)
Attachment #815462 - Flags: review?(ehsan)
Attachment #815466 - Flags: review?(ehsan)
Attachment #815467 - Flags: review?(ehsan)
Attachment #815468 - Flags: review?(ehsan)
Attachment #815469 - Flags: review?(ehsan)
Attachment #815461 - Flags: review?(ehsan) → review+
Attachment #815462 - Flags: review?(ehsan) → review+
Attachment #815463 - Flags: review?(ehsan) → review+
Attachment #815464 - Flags: review?(ehsan) → review+
Attachment #815465 - Flags: review?(ehsan) → review+
Attachment #815466 - Flags: review?(ehsan) → review+
Attachment #815467 - Flags: review?(ehsan) → review+
Attachment #815468 - Flags: review?(ehsan) → review+
Attachment #815469 - Flags: review?(ehsan) → review+
Attachment #815470 - Flags: review?(ehsan) → review+
This results in build failures under gcc 4.5.3.

toolkit/crashreporter/client/crashreporter.cpp:37:38: error: 'nullptr' was not declared in this scope
Depends on: 925818
For reference: This checkin http://hg.mozilla.org/mozilla-central/rev/fc9733225130 (from this bug here) uncovered  Bug 926083, which is about libxul link step failing on Windows with MSVC 2012. The important bit is "uncovered" here as I think the actual problem was already there before the change to nullptr.
Depends on: 926083
Apparently there are a number of developers using VS2012, especially in Metro land. They aren't taking too kindly to their builds being broken.

So, I backed out fc9733225130, which was isolated in comment #233 as the regression point. If we need to back out more changesets, a loud chorus of Windows developers would support that.

FWIW, I don't like backing out green changesets. But the reality is many Windows developers rely on VS2012. Bug 926705 has been filed to have RelEng deploy a VS2012 builder.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8bd4c964967
Some context here. Apparently way back in the day, VS2012 was required to build Firefox for Metro. So, many of our Metro developers have 2012 installed. They could possibly all be talked into installing 2010.
(In reply to Gregory Szorc [:gps] from comment #235)
> Some context here. Apparently way back in the day, VS2012 was required to
> build Firefox for Metro. So, many of our Metro developers have 2012
> installed. They could possibly all be talked into installing 2010.

Well yes but I thought later versions are required to build 64-bit Windows so probably still an issue for people trying to get that working.
(In reply to Gregory Szorc [:gps] from comment #235)
> Some context here. Apparently way back in the day, VS2012 was required to
> build Firefox for Metro. So, many of our Metro developers have 2012
> installed. They could possibly all be talked into installing 2010.

vs2012 is much nicer to use than vs2010, when the build isn't broken that is, so it would be a hard sell.
(In reply to Chris Pearce (:cpearce) from comment #238)
> (In reply to Gregory Szorc [:gps] from comment #235)
> > Some context here. Apparently way back in the day, VS2012 was required to
> > build Firefox for Metro. So, many of our Metro developers have 2012
> > installed. They could possibly all be talked into installing 2010.
> 
> vs2012 is much nicer to use than vs2010, when the build isn't broken that
> is, so it would be a hard sell.

Since vs2010 was also busted by that changeset, so it would be a hard sell indeed. :)
(In reply to :Ally Naaktgeboren from comment #239)
> Since vs2010 was also busted by that changeset, so it would be a hard sell
> indeed. :)

We build with vs2010 in automation *today*. So, something about the environments is different. Different Windows SDK versions or a conflicting SDK, perhaps? Who knows. Just another reason why we need reproducible build environments for developers.
(In reply to :Ally Naaktgeboren from comment #239)
> (In reply to Chris Pearce (:cpearce) from comment #238)
> > (In reply to Gregory Szorc [:gps] from comment #235)
> > > Some context here. Apparently way back in the day, VS2012 was required to
> > > build Firefox for Metro. So, many of our Metro developers have 2012
> > > installed. They could possibly all be talked into installing 2010.
> > 
> > vs2012 is much nicer to use than vs2010, when the build isn't broken that
> > is, so it would be a hard sell.
> 
> Since vs2010 was also busted by that changeset, so it would be a hard sell
> indeed. :)

I built just fine under vs2010 with that changeset, albeit under windows 7 but did include metro in the build.
Relanded the backed out changeset together with a workaround for bug 926083:

https://hg.mozilla.org/integration/mozilla-inbound/rev/73ac7e81f316
Attachment #821219 - Flags: review?(ehsan)
Attachment #821220 - Flags: review?(ehsan)
Attachment #821220 - Attachment description: Bug 784739 - Switch from NULL to nullptr in dom/plugins/base/ → Switch from NULL to nullptr in dom/plugins/base/
Attachment #821226 - Flags: review?(ehsan)
Attachment #821227 - Flags: review?(ehsan)
Attachment #821229 - Flags: review?(ehsan)
Attachment #821219 - Flags: review?(ehsan) → review+
Attachment #821220 - Flags: review?(ehsan) → review+
Attachment #821226 - Flags: review?(ehsan) → review+
Attachment #821227 - Flags: review?(ehsan) → review+
Attachment #821229 - Flags: review?(ehsan) → review+
Attachment #821230 - Flags: review?(ehsan) → review+
Attachment #821231 - Flags: review?(ehsan) → review+
...and gcc 4.5 is broken yet again. Please don't r+ patches like this unless they properly use mfbt/NullPtr.h, it's there for a reason.
Depends on: 930458
(In reply to comment #253)
> ...and gcc 4.5 is broken yet again. Please don't r+ patches like this unless
> they properly use mfbt/NullPtr.h, it's there for a reason.

Finding out where that include is actually needed requires somebody builds with gcc4.5 *on the platform that you are on*, which is not a supported build environment anyway.  Anyways, you have blanket r=me to add those includes anywhere that is needed until we figure out a better way (bug 930675 perhaps.)
Depends on: 931428
Attachment #822885 - Flags: review?(ehsan)
Attachment #822886 - Flags: review?(ehsan)
Attachment #822887 - Flags: review?(ehsan)
Attachment #822888 - Flags: review?(ehsan)
Attachment #822885 - Flags: review?(ehsan) → review+
Attachment #822886 - Flags: review?(ehsan) → review+
Attachment #822887 - Flags: review?(ehsan) → review+
Attachment #822888 - Flags: review?(ehsan) → review+
Attachment #822889 - Flags: review?(ehsan) → review+
Attachment #830265 - Flags: review?(ehsan)
Attachment #830266 - Flags: review?(ehsan)
Attachment #830267 - Flags: review?(ehsan)
Attachment #830268 - Flags: review?(ehsan)
Attachment #830265 - Flags: review?(ehsan) → review+
Attachment #830266 - Flags: review?(ehsan) → review+
Attachment #830267 - Flags: review?(ehsan) → review+
Attachment #830268 - Flags: review?(ehsan) → review+
Attachment #830269 - Flags: review?(ehsan) → review+
Attachment #8337262 - Flags: review?(ehsan)
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=2ca89a3958b3

(Note that the indexedDB changes are included in the "miscellaneous directories" changeset in the try push above.)
Attachment #8337264 - Flags: review?(ehsan)
Attachment #8337264 - Flags: feedback?(ethanhugg)
Comment on attachment 8337264 [details] [diff] [review]
Switch from NULL to nullptr in webrtc/signaling/

Review of attachment 8337264 [details] [diff] [review]:
-----------------------------------------------------------------

Seems to still build and work fine.  

I do wonder if we should be changing lines like this:
if (a != NULL)
to:
if (a)
instead of:
if (a != nullptr)
Attachment #8337264 - Flags: feedback?(ethanhugg) → feedback+
(In reply to Ethan Hugg [:ehugg] from comment #272)
> Seems to still build and work fine.  
> 
> I do wonder if we should be changing lines like this:
> if (a != NULL)
> to:
> if (a)
> instead of:
> if (a != nullptr)

I'd prefer if we did that in another bug, since this patch doesn't really change the weirdness of such code... :-)
Attachment #8337262 - Flags: review?(ehsan) → review+
Attachment #8337263 - Flags: review?(ehsan) → review+
Attachment #8337264 - Flags: review?(ehsan) → review+
Depends on: 953246
Depends on: 956042
Depends on: 938092
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=a486edf8b46d

This should cover the last remaining instances of NULL in our own code. Unless I have missed something, I think this bug can be (finally) closed.
Attachment #8355808 - Flags: review?(ehsan)
Attachment #8355790 - Flags: review?(ehsan) → review+
Attachment #8355808 - Flags: review?(ehsan) → review+
Thanks a lot for your effort here, Birunthan!

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ebd1e1a8197
https://hg.mozilla.org/integration/mozilla-inbound/rev/5954f2857de4
Whiteboard: [mentor=ehsan][lang=c++][leave open] → [mentor=ehsan][lang=c++]
https://hg.mozilla.org/mozilla-central/rev/7ebd1e1a8197
https://hg.mozilla.org/mozilla-central/rev/5954f2857de4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment on attachment 821220 [details] [diff] [review]
Switch from NULL to nullptr in dom/plugins/base/

Review of attachment 821220 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/base/npruntime.h
@@ +136,5 @@
>  
>  #define VOID_TO_NPVARIANT(_v)                                                 \
>  NP_BEGIN_MACRO                                                                \
>      (_v).type = NPVariantType_Void;                                           \
> +    (_v).value.objectValue = nullptr;                                         \

This file is a public API header. Various NPAPI plugins (which are not part of Mozilla) include this file.

Do you really want to require that anything using NPAPI must be built with C++11 support?
(In reply to Omair Majid from comment #280)
> Comment on attachment 821220 [details] [diff] [review]
> Switch from NULL to nullptr in dom/plugins/base/
> 
> Review of attachment 821220 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/plugins/base/npruntime.h
> @@ +136,5 @@
> >  
> >  #define VOID_TO_NPVARIANT(_v)                                                 \
> >  NP_BEGIN_MACRO                                                                \
> >      (_v).type = NPVariantType_Void;                                           \
> > +    (_v).value.objectValue = nullptr;                                         \
> 
> This file is a public API header. Various NPAPI plugins (which are not part
> of Mozilla) include this file.
> 
> Do you really want to require that anything using NPAPI must be built with
> C++11 support?

No, that was a mistake, thanks for catching it.  Fixed in https://hg.mozilla.org/integration/mozilla-inbound/rev/0691948a0abe.
I was told it's not possible to get it backported to 27.*. Any chance to have it in 28? Thanks
(In reply to Gabriele Giacone from comment #283)
> I was told it's not possible to get it backported to 27.*. Any chance to
> have it in 28? Thanks

What is it that you want backported exactly?
Flags: needinfo?(1o5g4r8o)
(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #284)
> (In reply to Gabriele Giacone from comment #283)
> > I was told it's not possible to get it backported to 27.*. Any chance to
> > have it in 28? Thanks
> 
> What is it that you want backported exactly?

Switching back to NULL for plugins.
https://hg.mozilla.org/mozilla-central/rev/0691948a0abe
Gnash plugin build against 27 is broken.
Flags: needinfo?(1o5g4r8o)
Note about this approval request: this patch doesn't change anything in the binaries that we ship, so it's 100% safe to backport anywhere.  It however changes the NPAPI headers we ship as part of the SDK which people use to build plugins. based on to make them C compatible again.  Without this patch, the plugin developers will need to fix this header themselves every time.

The mozilla-release approval is made so that this gets included in a potential next dot-release of 27 (in the SDK accompanying it to be more precise.)
Attachment #8380682 - Flags: approval-mozilla-release?
Attachment #8380682 - Flags: approval-mozilla-beta?
Attachment #8380682 - Flags: approval-mozilla-aurora?
The NPAPI headers should be unmodified copies of the files from the npapi-sdk project: https://code.google.com/p/npapi-sdk/

Is that still the case? (Plugin developers should be using those headers instead of the Mozilla SDK anyway).
(In reply to comment #287)
> The NPAPI headers should be unmodified copies of the files from the npapi-sdk
> project: https://code.google.com/p/npapi-sdk/
> 
> Is that still the case? (Plugin developers should be using those headers
> instead of the Mozilla SDK anyway).

Not sure what they use, but we definitely incorrectly modified the ones in our tree.
Comment on attachment 8380682 [details] [diff] [review]
Fix the npapi headers

No sign yet that there would be a 27.0.2 - approving for beta/aurora though and will keep the nom up for release in case the unexpected happens.
Attachment #8380682 - Flags: approval-mozilla-beta?
Attachment #8380682 - Flags: approval-mozilla-beta+
Attachment #8380682 - Flags: approval-mozilla-aurora?
Attachment #8380682 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/939571a11ebb
https://hg.mozilla.org/releases/mozilla-beta/rev/85053eae528b

Note that the "status-firefox28:fixed" status only applies to the very specific NPAPI follow-up fix in comment 282, not to the entirety of what landed over the course of this bug. It would have been preferable if this follow-up work be done in a separate bug for tracking purposes, but that's neither here nor there now, I guess.
This seems like it has a broad range of impact. Could someone please advise QA what, if any, areas of potential regression exist so we can test this before it is released?
This doesn't need manual QA.
Whiteboard: [mentor=ehsan][lang=c++] → [mentor=ehsan][lang=c++][qa-]
Comment on attachment 8380682 [details] [diff] [review]
Fix the npapi headers

It is in 29 and we are going to release it next week...
Attachment #8380682 - Flags: approval-mozilla-release? → approval-mozilla-release-
You need to log in before you can comment on or make changes to this bug.