Closed
Bug 795507
Opened 9 years ago
Closed 9 years ago
Remove the usages of PR_BEGIN_EXTERN_C and PR_END_EXTERN_C from the tree
Categories
(Core :: General, defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: ehsan, Assigned: isaac)
References
(Blocks 1 open bug)
Details
(Whiteboard: [mentor=ehsan][lang=c++])
Attachments
(2 files, 1 obsolete file)
1018 bytes,
text/plain
|
Details | |
17.82 KB,
patch
|
ehsan
:
review+
|
Details | Diff | Splinter Review |
Inside headers, these macros should be replaced by something like this: #ifdef __cplusplus extern "C" { #endif // ... some code #ifdef __cplusplus } #end Inside .cpp files, the #ifdef dance above is not needed, because, well, we'll be compiling in C++ mode. :-)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → isaac.aggrey
Comment 1•9 years ago
|
||
Try run for 5d7ef44fa6da is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=5d7ef44fa6da Results (out of 16 total builds): exception: 16 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/isaac.aggrey@gmail.com-5d7ef44fa6da
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #666387 -
Flags: review?(ehsan)
Assignee | ||
Comment 4•9 years ago
|
||
Ignore the try run results posted above. I cancelled my earlier try run because I keep prematurely pushing to try before building/testing on my machine to catch silly mistakes; fortunately, I cancel them before they run more than a few . Try results will be at https://tbpl.mozilla.org/?tree=Try&rev=60ff6f4ebdb5
Comment 5•9 years ago
|
||
Try run for 60ff6f4ebdb5 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=60ff6f4ebdb5 Results (out of 66 total builds): exception: 25 success: 32 failure: 9 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/isaac.aggrey@gmail.com-60ff6f4ebdb5
Reporter | ||
Comment 6•9 years ago
|
||
Comment on attachment 666387 [details] [diff] [review] Remove PR_BEGIN_EXTERN_C and PR_END_EXTERN_C from tree Review of attachment 666387 [details] [diff] [review]: ----------------------------------------------------------------- ::: dbm/include/mcom_db.h @@ +415,5 @@ > #endif > > +#ifdef __cplusplus > +} > +#end I believe this should be #endif here, as try server results indicate. :-)
Attachment #666387 -
Flags: review?(ehsan)
Assignee | ||
Comment 7•9 years ago
|
||
Oops, I really should be looking at my diffs since I thought I pushed the corrected version. I made the incorrect assumption that my revert in source control and re-application of the script caught everything. Another lesson learned! Thanks, Ehsan.
Attachment #666387 -
Attachment is obsolete: true
Attachment #666419 -
Flags: review?(ehsan)
Reporter | ||
Comment 8•9 years ago
|
||
Comment on attachment 666419 [details] [diff] [review] Remove PR_BEGIN_EXTERN_C and PR_END_EXTERN_C from tree (fixed) Review of attachment 666419 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks!
Attachment #666419 -
Flags: review?(ehsan) → review+
Reporter | ||
Comment 9•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d1a365a3320
Target Milestone: --- → mozilla18
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2d1a365a3320
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Reporter | ||
Updated•9 years ago
|
Blocks: dieprtypesdie
Comment 11•9 years ago
|
||
Sorry guys, you overlooked that directory mozilla/dbm is owned by the NSS library, and you must not change it except by upgrading to the new NSS release. The upstream for any changes to this directory is the NSS project. (I guess we'll have to make that clearer by adding a file to that directory....?) How shall we proceed? Either you try to get review to the changes to file dbm/include/mcom_db.h TODAY and we check it in to upstream NSS, or it should be backed out from mozilla-central. The reason for the urge is that we're about to create a NSS 3.14 release (by thursday).
Comment 12•9 years ago
|
||
Note that I'll backout the mozilla/dbm portion if nothing happens by tomorrow (it will be backed out / overwritten automatically anyway, as soon as we upgrade to 3.14 final release).
Updated•9 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 13•9 years ago
|
||
Sigh... I'm not sure how easy it would be to get reviews in the upstream project. My experience on the review turn-around time in the past has mostly been weeks, not days. So I guess I'll backout those parts. :(
Reporter | ||
Comment 14•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/830fccc0509c
Comment 15•9 years ago
|
||
Ehsan, there should be no Mozilla C++ code including these DBM headers anyway, so this should not affect bug 796941 anyway. So, while I'd be happy to r+ your patch and check it into the NSS tree, I don't think it is necessary.
Comment 16•9 years ago
|
||
Thank you very much for backing out from mozilla-central! I propose to simply file a new NSS bug, attach the patch, have bsmith r+ it, and we'll check it in to NSS. thanks
Reporter | ||
Comment 17•9 years ago
|
||
Sure, did that in bug 799717.
Comment 18•9 years ago
|
||
(In reply to Ehsan Akhgari [:ehsan] from comment #14) > Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/830fccc0509c Merge of backout: https://hg.mozilla.org/mozilla-central/rev/830fccc0509c
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•