375985 - Add compile time assertions to NSPR

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Jonathan Watt [:jwatt]]

Attachment: patch

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.

Comment 1

[Jeff Walden [:Waldo]]

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.
*/

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch with improved comment

Good comments. Here's an update.

Comment 3

[Wan-Teh Chang]

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.

Comment 4

[Jonathan Watt [:jwatt]]

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?

Comment 5

[Wan-Teh Chang]

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.

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: patch with even better comment

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.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch with even better comment

Comment 8

[Jeff Walden [:Waldo]]

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

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: patch with better spelling ;-)

Thanks Jeff.

Comment 10

[Wan-Teh Chang]

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.

Comment 11

[Jonathan Watt [:jwatt]]

Fixed. Checked in a couple of days ago.

Comment 12

[Jeff Walden [:Waldo]]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

Should probably document that this should not be used as-is in headers.  See bug 381236.

Comment 14

[Wan-Teh Chang]

Attachment: Define PR_STATIC_ASSERT as extern declaration rather than typedef

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.

Comment 15

[Wan-Teh Chang]

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