(dormant account) Reporter
	Description • 15 years ago

See sixgill.org for more info.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 2 • 15 years ago

Comment on attachment 422913 [details] [diff] [review]
This patch adds macros for sixgill annotation attributes to nsDebug.h

Benjamin, what do you think of these macros? I wonder if we want the macro names to be more like NS_XYZ.

	Benjamin Smedberg
	Comment 3 • 15 years ago

Comment on attachment 422913 [details] [diff] [review]
This patch adds macros for sixgill annotation attributes to nsDebug.h

Please don't introduce identifiers that start with two underscores... those are reserved for compilers.

I wonder how generic we want to make this: if this is going to be specific to mozilla, I guess we should use NS_assert_static. If we want this to be sixgill-specific, then maybe have a sixgill header and use sg_assert_static?

I don't have a strong preference except that I don't want naming conflicts with other stuff, so prefixes are probably necessary.

	Brian Hackett [Laid off!] Assignee
	Comment 4 • 15 years ago

(In reply to comment #3)
> I wonder how generic we want to make this: if this is going to be specific to
> mozilla, I guess we should use NS_assert_static. If we want this to be
> sixgill-specific, then maybe have a sixgill header and use sg_assert_static?

Another alternative is to use macros that are not specific either to mozilla or sixgill.  While sixgill is the only tool using these attributes, other tools should be able to use them in the future (currently the annotation parsing is done by the sixgill frontend plugin; I'd like to push this into gcc itself).

STC_ASSERT   // STATIC => STC_
STC_PRECONDITION
STC_PRECONDITION_ASSUME
...

	Brendan Eich [:brendan]
	Comment 5 • 15 years ago

STATIC_ beats STC_ or other cybercrud, at least in Mozilla code. I hope gcc folks feel the same way (I would expect so, but my last gcc patch was in the '90s).

/be

	Brian Hackett [Laid off!] Assignee
	Comment 6 • 15 years ago

This patch uses STATIC_UPPERCASE macros for all the annotations, and does not make any '__' identifiers.  I've tested this on my x86_64 machine and it works both with and without the sixgill plugin.

	Brian Hackett [Laid off!] Assignee
	Comment 7 • 15 years ago

This patch allows sixgill to check NS_ASSERTION macros when doing a release build (preferable (?) so we don't analyze debug code).  Assertions referring to variables/functions that only exist in debug code will be ignored.

	dwitte@gmail.com
	Comment 8 • 15 years ago

Comment on attachment 438884 [details] [diff] [review]
Updated patch that checks NS_ASSERTION in release and debug builds

>diff -r 9614b3d45ff4 xpcom/glue/nsDebug.h

>+/* Used to make identifiers for assert/assume annotations in a function. */
>+#define STATIC_PASTE2(X,Y) X ## Y
>+#define STATIC_PASTE1(X,Y) STATIC_PASTE2(X,Y)

What's the purpose of having two?

Otherwise looks great. In general, debug code should be a superset of release code, but there's nothing forcing that -- so I agree, we definitely want to be able to analyze release code with the assistance of static assertions.

r=dwitte

	Jeff Walden [:Waldo]
	Comment 9 • 15 years ago

You need two levels of indirection if one of the tokens being pasted might be __LINE__.

	dwitte@gmail.com
	Comment 10 • 15 years ago

> You need two levels of indirection if one of the tokens being pasted might be
> __LINE__.

Hilarious. :)

If this is ready to land, I'll stick it in my queue.

	dwitte@gmail.com
	Comment 11 • 15 years ago

Comment on attachment 438884 [details] [diff] [review]
Updated patch that checks NS_ASSERTION in release and debug builds

http://hg.mozilla.org/mozilla-central/rev/ff91905fba1b

	Brian Hackett [Laid off!] Assignee
	Comment 12 • 15 years ago

Argh, it looks like this checkin has carriage returns at the end of every line (I think my Windows machine inserted them when I posted the patch here).  Is there an easy way to get rid of these?

	dwitte@gmail.com
	Comment 13 • 15 years ago

Indeed it did. I'll strip 'em out.

	dwitte@gmail.com
	Comment 14 • 15 years ago

Done.

http://hg.mozilla.org/mozilla-central/rev/765dd059291f

	Brian Hackett [Laid off!] Assignee
	Comment 15 • 15 years ago

Looks like things are working good now.  Thanks!