GCC >=4.0 on the Mozilla code base spews thousands of warnings about applying offsetof() to non-POD types. On some files there are so many of these warnings that you lose actual error messages past the top of your scrollback buffer. Perhaps the code should be changed someday, but most of the cases I've actually looked at would be very hard to change and are unlikely to cause actual trouble. Thus, it would be nice if we could get -Wno-invalid-offsetof added to the C++ command line flags when using these versions of GCC.

Yeah, I think we should do this.

Comment on attachment 334283 [details] [diff] [review] supress offsetof warnings, rev. 1 r=dbaron, although I think it would be better to put the whole thing inside a test for GNU_CC. Some compilers might handle invalid options with something other than an error. (I'm not sure what _COMPILER_PREFIX is for, though.) And perhaps this should live somewhere closer to the -Wno-long-long tests? Or do we just not worry about the order of things inside configure?

Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/41d9d32ab5a7 I moved the check into a GNU_CXX block... I didn't move it down with long-long because that was part of a block of code that reads configure arguments and this is not pref-controlled.

(In reply to comment #0) > most of the cases I've actually looked at would be very hard to change JFTR, try template<class T> struct offsetof_helper { static T instance; }; #define offsetof(T, M) (\ reinterpret_cast<char *>(&(offsetof_helper<T>::instance.M) - \ reinterpret_cast<char *>(&(offsetof_helper<T>::instance)) Unlike casting a random address to a pointer type the compiler really does know exactly which type you're using so by my wooly thinking it should be safe for virtally (!) any class you throw at it.

... wow, that actually works (with g++ 4.3.1); I was sure it was going to generate external references to the never-actually-defined object offsetof_helper<X>::instance! (You need two close parens right after "instance.M" and three at the very end, by the way.) Before doing any such thing I would want to be very sure that it worked correctly with all relevant compilers (in particular I understand MSVC is still not great with templates) and I'd want an opinion from a C++ expert that it wasn't going to blow up in our faces.

I don't think doing that is necessary or desirable, because compilers may emit symbols to the nonexistent object. If we really wanted to avoid offsetof, couldn't we use something like this: #define safe_offsetof(T, M) \ (reinterpret_cast<char*>(&(((T*)0)->M)) - reinterpret_cast<char*>(0))

> &(((T*)0)->M) That's the construct that triggers the obnoxious warnings from recent g++; we're already using that or something very like it. (The precise text of the warning is warning: invalid access to non-static data member ‘B::aye’ of NULL object warning: (perhaps the ‘offsetof’ macro was used incorrectly) )

Oh, well then use the following construct, which I've used to good effect in nsISupportsImpl.h: http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#434 #define safe_offsetof(T, M) \ (reinterpret_cast<char*>(&(((T*)1000)->M)) - reinterpret_cast<char*>(1000))

... g++ doesn't complain about that, but I think it probably *should*.

(In reply to comment #5) > (You need two close parens right after > "instance.M" and three at the very end, by the way.) Sorry, my fault for not cut-n-pasting. (I actually had fewer open parens.)

The patch as checked in (comment 3) seems wrong. Autoconf generates ac_compile='${CC-cc} -c $CFLAGS $CPPFLAGS conftest.$ac_ext 1>&5' ac_link='${CC-cc} -o conftest${ac_exeext} $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS 1>&5' (without line breaks) so the flags that is temporarily added to CXXFLAGS is never actually tested for. I don't find a copy of the autoconf manual for version 2.13 any more, but I think it would be better to use either CFLAGS (as in the reviewed version of the patch) or CPPFLAGS. Hmm, or did you mean to use AC_LANG_CPLUSPLUS but forgot to add it?

Yeah, forgot the AC_LANG_CPLUSPLUS at some point... that is now fixed: http://hg.mozilla.org/mozilla-central/index.cgi/rev/ccf420d6618b