Remove the usages of PR_BEGIN_EXTERN_C and PR_END_EXTERN_C from the tree

RESOLVED FIXED in mozilla18

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Away for a while, Assigned: isaac)

Tracking

(Blocks: 1 bug)

Trunk
mozilla18
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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

5 years ago
Assignee: nobody → isaac.aggrey

Comment 1

5 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

5 years ago
Created attachment 666386 [details]
Script to replace PR_BEGIN_EXTERN_C and PR_END_EXTERN_C
(Assignee)

Comment 3

5 years ago
Created attachment 666387 [details] [diff] [review]
Remove PR_BEGIN_EXTERN_C and PR_END_EXTERN_C from tree
Attachment #666387 - Flags: review?(ehsan)
(Assignee)

Comment 4

5 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

5 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

5 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

5 years ago
Created attachment 666419 [details] [diff] [review]
Remove PR_BEGIN_EXTERN_C and PR_END_EXTERN_C from tree (fixed)

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

5 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

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d1a365a3320
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/2d1a365a3320
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
Blocks: 796941

Comment 11

5 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

5 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

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 13

5 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

5 years ago
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/830fccc0509c
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

5 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

5 years ago
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
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.