The default bug view has changed. See this FAQ.

NSS public header files should be C++ safe

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P3
minor
RESOLVED FIXED
15 years ago
10 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Julien Pierre)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

1.11 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
NSS public header files should be able to be safely included
by a C++ file without the need for an extern "C" { } block.

I just noticed that several NSS headers are included by PSM
files (C++) in extern "C" blocks:

pkcs11.h
pkcs12.h
p12plcy.h
pk11pqg.h
secdert.h
keydbt.h
crmf.h

We should get them fixed.
(Assignee)

Comment 1

15 years ago
Wan-Teh,

Did you try to remove the extern "C" and build PSM ?
I know there have been issues in the past with headers' C++ safety but many got
resolved. Usually a temporary workaround was to put extern "C" before fixes were
made. That was certainly the case in the web server, and those workarounds were
never removed since their presence did not harm anything in the build process. 
(Reporter)

Comment 2

15 years ago
No, I didn't try to remove the extern "C" and see if it works.

I filed this bug so that I don't forget about this problem.
It is low priority and has an easy workaround.
Severity: normal → minor
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → 3.4
(Reporter)

Comment 3

15 years ago
Changed the QA contact to Bishakha.
QA Contact: sonja.mirtitsch → bishakhabanerjee
(Reporter)

Comment 4

15 years ago
Set target milestone to NSS 3.5.
Target Milestone: 3.4 → 3.5
(Reporter)

Updated

15 years ago
Target Milestone: 3.5 → 3.7
(Reporter)

Comment 5

15 years ago
Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.
Target Milestone: 3.7 → 3.8
Remove target milestone of 3.8, since these bugs didn't get into that release.
Target Milestone: 3.8 → ---
QA Contact: bishakhabanerjee → jason.m.reid
QA Contact: jason.m.reid → libraries
(Assignee)

Updated

10 years ago
Assignee: wtc → julien.pierre.boogz
Status: ASSIGNED → NEW
(Assignee)

Comment 7

10 years ago
I checked all the header files named above. keydbt.h no longer exists. All the others have been fixed and are C++-safe, except for secdert.h which has some data symbols outside that are not in a SEC_BEGIN_PROTOS / SEC_END_PROTOS block . Somehow I doubt this really poses much of a problem because the data symbols wouldn't even be visible on Windows, so the name mangling would probably not even come into play. But just for consistency, I will provide a patch for that header.
(Assignee)

Comment 8

10 years ago
Created attachment 271781 [details] [diff] [review]
Add SEC_BEGIN_PROTOS / SEC_END_PROTOS
Attachment #271781 - Flags: review?(nelson)
(Assignee)

Updated

10 years ago
Target Milestone: --- → 3.12
Comment on attachment 271781 [details] [diff] [review]
Add SEC_BEGIN_PROTOS / SEC_END_PROTOS

Doesn't the ``extern "C" { '' need to enclose all the typedefs and struct
type declarations, to prevent their type names from being mangled, and
to prevent the structs from being treated as public classes?  
If so, then the BEGIN_PROTOS is in the wrong place.
If not, then this is OK, but I'd prefer that the BEGIN_PROTOS be put at 
the front of the file anyway.
(Assignee)

Comment 10

10 years ago
Nelson,

extern "C" doesn't have any effect on typedefs and structs, it is only for function and variable symbol names, but it is harmless to move it up.
Comment on attachment 271781 [details] [diff] [review]
Add SEC_BEGIN_PROTOS / SEC_END_PROTOS

r=nelson
Move BEGIN_PROTOS up or not as you see fit.
Attachment #271781 - Flags: review?(nelson) → review+
(Reporter)

Comment 12

10 years ago
Right.  I observed that NSS headers have been careful to
exclude typedefs, structs, and macros from extern "C".
So it is a good idea to preserve that convention.
(Assignee)

Comment 13

10 years ago
Thanks for the reviews. I checked in the patch as-is to the trunk.

Checking in secdert.h;
/cvsroot/mozilla/security/nss/lib/util/secdert.h,v  <--  secdert.h
new revision: 1.3; previous revision: 1.2
done
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 14

10 years ago
This patch breaks compilation of PSM-with-NSS-3.12

nsKeygenHandler.cpp
../../../../dist/public/nss/secdert.h:163: error: expected constructor, destructor, or type conversion before ‘extern’
../../../../dist/include/system_wrappers/secdert.h:4: error: ‘#pragma’ is not allowed here

It was necessary to add
  #include "seccomon.h"
to file secdert.h
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 15

10 years ago
The compilation error was addressed in bug 388120, setting bug back to resolved.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Depends on: 388120
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.