Closed Bug 375985 Opened 13 years ago Closed 13 years ago

Add compile time assertions to NSPR

Categories

(NSPR :: NSPR, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jwatt, Assigned: jwatt)

References

()

Details

Attachments

(3 files, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
It would be great if NSPR supported compile time assertions. I have some code I'm working on that could really use them.

In bug 327896 Igor Bukanov implemented compile time assertions for JS. The define can be seen at:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsutil.h&rev=3.13&mark=70-80#70

Here's a patch to copy this code to NSPR. I'm not familiar with NSPR review process, so please let me know about that.
Attachment #260122 - Flags: review?(wtchang)
A comment explaining why IMPL and IMPL2 are both needed would be nice.  I recall it having something to do with either line number or file name, one of the two, but don't remember the details.  Also, I suspect you'll want to match existing comment style, so:

/*
** Comment goes here and
** extends to here.
*/
Good comments. Here's an update.
Attachment #260122 - Attachment is obsolete: true
Attachment #260252 - Flags: review?(wtchang)
Attachment #260122 - Flags: review?(wtchang)
Comment on attachment 260252 [details] [diff] [review]
patch with improved comment

r=wtc.  This is a cool trick.  I found that Boost also
has a BOOST_STATIC_ASSERT macro for this purpose.

Does any compiler support compile-time assertions
natively?  I seem to remember that GCC does, but can't
find the name of that.  A built-in compile-time
assertion would give a better error message than
the error message

  foo.c(1) : error C2118: negative subscript

that Visual C++ 2005 gives me.
Attachment #260252 - Flags: review?(wtchang) → review+
Yeah, unfortunately I can't claim credit for it. ;-)

Googling BOOST_STATIC_ASSERT I see that it would require copyright and license headers to be copied over, so I didn't look at the code for now. I'm not sure if you're suggesting using their stuff instead of this patch?

I use VS 2005 too, which I'm fairly sure does not natively support compile-time assertions. I don't know about other compilers such as gcc I'm afraid, but I found http://gcc.gnu.org/ml/gcc-patches/2001-08/msg00006.html googling. I agree the error message could be better, but it should do the job. ;-)

I'm a bit confused by the r+ along with comments. Should I check in now, or do you want further improvements or additional review?
Jonathan,

I will check in this patch for you.  The improvement (use compiler's
built-in compile-time assertions) is not mandatory.

Sorry I wasn't clear about my BOOST_STATIC_ASSERT comment.  I meant to
point out that 1) this trick is well-known, and 2) the (new) name
STATIC_ASSERT is good because it's consistent with other projects
(there was a change of name to the original JavaScript patch).  I
didn't mean to suggest any change or use their implementation.
Attached patch patch with even better comment (obsolete) — Splinter Review
That's great, thanks for the explanation.

One of my fiends still didn't understand from the comment in the previous patch how the macro indirection works. Here's a patch with what I think is better text.
Attached patch patch with even better comment (obsolete) — Splinter Review
Attachment #260275 - Attachment is obsolete: true
Comment on attachment 260276 [details] [diff] [review]
patch with even better comment

>Index: mozilla/nsprpub/pr/include/prerror.h

>+** macro indirection. IMPL is required to get macro-expantion of __LINE__ to

expansion

>+** it's integer value so that IMPL2 will stringify the number, not "__LINE__".

its
Thanks Jeff.
Attachment #260276 - Attachment is obsolete: true
The double macro expansion trick is due to an esoteric proporty
of the C preprocessor.  It is usually used with # or ##.  The
C preprocessor only expands the macros once, so when you
use # or ## with a macro and wants # or ## to apply to the
macro's value rather than the macro's name, you need to use
the double macro expansion trick.
Fixed. Checked in a couple of days ago.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.7
http://developer.mozilla.org/en/docs/PR_STATIC_ASSERT

I basically stole the stuff for PR_ASSERT from the reference (had to migrate that at the same time) and tweaked it to note that the assertion *always* is checked and that PR_STATIC_ASSERT is valid only in syntactic locations where typedefs are allowed.

Also, feel free to make changes to the following other docs I migrated/stubbed so that PR_STATIC_ASSERT is linked from somewhere and so that it can refer to PR_ASSERT for non-compile-time expressions.

http://developer.mozilla.org/en/docs/PR_ASSERT
http://developer.mozilla.org/en/docs/NSPR_API_Reference:Logging
Should probably document that this should not be used as-is in headers.  See bug 381236.
1. Move the definition of PR_STATIC_ASSERT from perror.h
   to prlog.h, where the related PR_ASSERT macro is defined.

2. Define PR_STATIC_ASSERT as an extern declaration rather
   rather than a typedef.  See JavaScript bug 381236 for
   JS_STATIC_ASSERT.
Attachment #266088 - Flags: review?(igor)
Attachment #266088 - Flags: review?(igor) → review+
Comment on attachment 266088 [details] [diff] [review]
Define PR_STATIC_ASSERT as extern declaration rather than typedef

I checked in this patch on the NSPR trunk (NSPR 4.7)
and the NSPRPUB_PRE_4_2_CLIENT_BRANCH for the Mozilla
trunk (1.9 pre-release).

Checking in prerror.h;
/cvsroot/mozilla/nsprpub/pr/include/prerror.h,v  <--  prerror.h
new revision: 3.14; previous revision: 3.13
done
Checking in prlog.h;
/cvsroot/mozilla/nsprpub/pr/include/prlog.h,v  <--  prlog.h
new revision: 3.15; previous revision: 3.14
done

Checking in prerror.h;
/cvsroot/mozilla/nsprpub/pr/include/prerror.h,v  <--  prerror.h
new revision: 3.11.4.3; previous revision: 3.11.4.2
done
Checking in prlog.h;
/cvsroot/mozilla/nsprpub/pr/include/prlog.h,v  <--  prlog.h
new revision: 3.13.2.2; previous revision: 3.13.2.1
done
You need to log in before you can comment on or make changes to this bug.