Mike Hommey [:glandium] Reporter
	Description • 8 years ago

Build fails on try with GCC 4.8.5 (with --enable-warnings-as-errors):

/home/worker/workspace/build/src/accessible/atk/AtkSocketAccessible.cpp:25:489: error: typedef '_GStaticAssertCompileTimeAssertion_0' locally defined but not used [-Werror=unused-local-typedefs]
/home/worker/workspace/build/src/accessible/atk/AtkSocketAccessible.cpp:25:691: error: typedef '_GStaticAssertCompileTimeAssertion_1' locally defined but not used [-Werror=unused-local-typedefs]
/home/worker/workspace/build/src/accessible/atk/AtkSocketAccessible.cpp:25:1496: error: typedef '_GStaticAssertCompileTimeAssertion_2' locally defined but not used [-Werror=unused-local-typedefs] 

https://treeherder.mozilla.org/logviewer.html#?job_id=15989654&repo=try

	Mike Hommey [:glandium] Reporter
	Comment 1 • 8 years ago

Fails equally with GCC 4.9.3.

	Marco Zehe (:MarcoZ)
	Comment 2 • 8 years ago

Trev, is this something you worked on recently?

	Trevor Saunders (:tbsaunde)
	Comment 3 • 8 years ago

(In reply to Mike Hommey [:glandium] from comment #0)
> Build fails on try with GCC 4.8.5 (with --enable-warnings-as-errors):
> 
> /home/worker/workspace/build/src/accessible/atk/AtkSocketAccessible.cpp:25:
> 489: error: typedef '_GStaticAssertCompileTimeAssertion_0' locally defined
> but not used [-Werror=unused-local-typedefs]
> /home/worker/workspace/build/src/accessible/atk/AtkSocketAccessible.cpp:25:
> 691: error: typedef '_GStaticAssertCompileTimeAssertion_1' locally defined
> but not used [-Werror=unused-local-typedefs]
> /home/worker/workspace/build/src/accessible/atk/AtkSocketAccessible.cpp:25:
> 1496: error: typedef '_GStaticAssertCompileTimeAssertion_2' locally defined
> but not used [-Werror=unused-local-typedefs] 

So this is coming from the G_DEFINE_TYPE_EXTENDED macro, which I believe comes from glib / gobject.  I can't reproduce this locally with gcc 5, so I'm guessing its fixed in newer gobject headers.  Its probably easiest to just #pragma diagnostic our way to victory.

	Chris Peterson [:cpeterson]
	Comment 4 • 8 years ago

Bug 1117259 added -Wno-unused-local-typedef to configure.in [1], but "-Wunused-local-typedef" is clang's warning name. gcc's name is "-Wunused-local-typedefs" (plural) [2]. We should use -Wunused-local-typedefs because clang accepts it as an alias [3].

Also, since both clang and gcc 4.7+ accept "-Wno-unused-local-typedefs", we no longer need the MOZ_CXX_SUPPORTS_WARNING check (assuming our minimum gcc version is 4.7).

[1] https://mxr.mozilla.org/mozilla-central/source/configure.in#1591
[2] https://gcc.gnu.org/onlinedocs/gcc-4.8.0/gcc/Warning-Options.html
[3] https://github.com/llvm-mirror/clang/blob/master/include/clang/Basic/DiagnosticGroups.td#L688

	Trevor Saunders (:tbsaunde)
	Comment 5 • 8 years ago

(In reply to Chris Peterson [:cpeterson] from comment #4)
> Bug 1117259 added -Wno-unused-local-typedef to configure.in [1], but
> "-Wunused-local-typedef" is clang's warning name. gcc's name is
> "-Wunused-local-typedefs" (plural) [2]. We should use
> -Wunused-local-typedefs because clang accepts it as an alias [3].

It seems to me it would be better to keep this warning enabled especially if this is the only problem case.

	(no longer active)
	Comment 6 • 8 years ago

Yeah this is a typo.  I have a fix in bug 1243604.

	Steve Fink [:sfink] [:s:] Assignee
	Comment 7 • 8 years ago

There seems to be a fair amount of discussion about the right way to fix this (wait for upstream etc.) but I really need to upgrade gcc, so I'd like to just suppress this warning for now.

	Trevor Saunders (:tbsaunde)
	Comment 8 • 8 years ago

Comment on attachment 8713761 [details] [diff] [review]
Prevent G_DEFINE_TYPE_EXTENDED macro from producing a fatal warning

>-if CONFIG['CLANG_CXX']:
>-    # Suppress clang warning about unused function from gobject's RTTI macros.
>-    CXXFLAGS += ['-Wno-unused-function']

aren't you accidentally reenablingthis warning?

>+if CONFIG['CLANG_CXX'] or CONFIG['GNU_CXX']:
>+    # Used in G_DEFINE_TYPE_EXTENDED macro, probably fixed in newer glib /
>+    # gobject headers. See bug 1243331 comment 3.
>+    CXXFLAGS += ['-Wno-unused-local-typedefs']

I'd prefer #pragma just around the macro use, but I'm fine with this if you explain the unused function issue.

	Chris Peterson [:cpeterson]
	Comment 9 • 8 years ago

Comment on attachment 8713761 [details] [diff] [review]
Prevent G_DEFINE_TYPE_EXTENDED macro from producing a fatal warning

Review of attachment 8713761 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/atk/moz.build
@@ +51,5 @@
>      CXXFLAGS += CONFIG['MOZ_DBUS_CFLAGS']
>  
>  include('/ipc/chromium/chromium-config.mozbuild')
>  
> +if CONFIG['CLANG_CXX'] or CONFIG['GNU_CXX']:

You can just use `if CONFIG['GNU_CXX']:` because clang defines both CLANG_CXX and GNU_CXX. To add clang-only flags, we have to check `if CONFIG['CLANG_CXX'] and not CONFIG['GNU_CXX']:`.

	Steve Fink [:sfink] [:s:] Assignee
	Comment 10 • 8 years ago

Uh, yeah, thanks! I really ought to read patches before submitting them for review.

	Trevor Saunders (:tbsaunde)
	Comment 11 • 8 years ago

(In reply to Chris Peterson [:cpeterson] from comment #9)
> Comment on attachment 8713761 [details] [diff] [review]
> Prevent G_DEFINE_TYPE_EXTENDED macro from producing a fatal warning
> 
> Review of attachment 8713761 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/atk/moz.build
> @@ +51,5 @@
> >      CXXFLAGS += CONFIG['MOZ_DBUS_CFLAGS']
> >  
> >  include('/ipc/chromium/chromium-config.mozbuild')
> >  
> > +if CONFIG['CLANG_CXX'] or CONFIG['GNU_CXX']:
> 
> You can just use `if CONFIG['GNU_CXX']:` because clang defines both
> CLANG_CXX and GNU_CXX. To add clang-only flags, we have to check `if
> CONFIG['CLANG_CXX'] and not CONFIG['GNU_CXX']:`.

I'd say its better to explicitly check for gcc or clang, and that'll take care of things like clang-cl where GNUCXX is not defined.

Of course in this particular case we could probably do without the check all together, I kind of doubt anybody tries to compile this code with something other than gcc or clang.

	Trevor Saunders (:tbsaunde)
	Comment 12 • 8 years ago

Comment on attachment 8713829 [details] [diff] [review]
Prevent G_DEFINE_TYPE_EXTENDED macro from producing a fatal warning

I guess this technically build config, but I'll claim its a11y enough that its not unreasonable for me to review, and save glandium 5 seconds.

	Pulsebot
	Comment 13 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e187d1b2244f

	Carsten Book [:Tomcat]
	Comment 14 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e187d1b2244f