Closed Bug 795507 Opened 7 years ago Closed 7 years ago

Remove the usages of PR_BEGIN_EXTERN_C and PR_END_EXTERN_C from the tree

Categories

(Core :: General, defect)

x86_64
Linux
defect
Not set

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)

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: nobody → isaac.aggrey
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
Attachment #666387 - Flags: review?(ehsan)
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
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
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)
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)
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d1a365a3320
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/2d1a365a3320
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
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).
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.  :(
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.
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
Sure, did that in bug 799717.
(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: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.