Closed Bug 1232772 Opened 8 years ago Closed 8 years ago

suppress numerous clang-style warnings when using clang-cl

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox46 fixed)

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(1 file, 1 obsolete file)

Suppressing these warnings significantly cuts down the warning spam on
clang-cl builds.
Attachment #8698577 - Flags: review?(mh+mozilla)
Blocks: winclang, 1042132
I guess we should add these to js/src/configure.in too...
Now with js/src/ modifications.
Attachment #8698586 - Flags: review?(mh+mozilla)
Attachment #8698577 - Attachment is obsolete: true
Attachment #8698577 - Flags: review?(mh+mozilla)
Comment on attachment 8698586 [details] [diff] [review]
suppress numerous clang-style warnings when using clang-cl

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

::: configure.in
@@ +2287,5 @@
> +            CFLAGS="$CFLAGS -Wno-invalid-noreturn"
> +            CXXFLAGS="$CXXFLAGS -Wno-invalid-noreturn"
> +            # Missing |override| on virtual function declarations isn't
> +            # something that MSVC currently warns about.
> +            CXXFLAGS="$CXXFLAGS -Wno-inconsistent-missing-override"

Is that something we're hitting on system headers or windows platform code? Because IIRC, we're supposed to use override consistently in Gecko code.

@@ +2290,5 @@
> +            # something that MSVC currently warns about.
> +            CXXFLAGS="$CXXFLAGS -Wno-inconsistent-missing-override"
> +            # We declare |operator delete(void*)| differently than MSVC,
> +            # which causes a raft of warnings if we leave this enabled.
> +            CXXFLAGS="$CXXFLAGS -Wno-implicit-exception-spec-mismatch"

Can you provide more detail here, because this might actually be important.
Attachment #8698586 - Flags: review?(mh+mozilla) → feedback+
(In reply to Mike Hommey [:glandium] from comment #4)
> Comment on attachment 8698586 [details] [diff] [review]
> suppress numerous clang-style warnings when using clang-cl
> 
> Review of attachment 8698586 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: configure.in
> @@ +2287,5 @@
> > +            CFLAGS="$CFLAGS -Wno-invalid-noreturn"
> > +            CXXFLAGS="$CXXFLAGS -Wno-invalid-noreturn"
> > +            # Missing |override| on virtual function declarations isn't
> > +            # something that MSVC currently warns about.
> > +            CXXFLAGS="$CXXFLAGS -Wno-inconsistent-missing-override"
> 
> Is that something we're hitting on system headers or windows platform code?
> Because IIRC, we're supposed to use override consistently in Gecko code.

These appear to be coming from Gecko graphics headers and Gecko accessibility headers.  Some of them are fixable; some of them come from macros defined by the Windows IDL compiler, so are not easily fixable.

We could fix all the warnings, but they're just going to come back, seeing as how nothing is checking for this with our normal Windows builds...

> @@ +2290,5 @@
> > +            # something that MSVC currently warns about.
> > +            CXXFLAGS="$CXXFLAGS -Wno-inconsistent-missing-override"
> > +            # We declare |operator delete(void*)| differently than MSVC,
> > +            # which causes a raft of warnings if we leave this enabled.
> > +            CXXFLAGS="$CXXFLAGS -Wno-implicit-exception-spec-mismatch"
> 
> Can you provide more detail here, because this might actually be important.

The declaration from <new> says:

void __CRTDECL operator delete(void *) _THROW0();

and the warning says:

function previously declared with an explicit exception specification redeclared with an implicit exception specification

Hm.  I think that means our clang isn't turning off exceptions, because <xstddef> only defines THROW0 to something if _HAS_EXCEPTIONS.  Maybe that's not a good thing...
There's also a lot of inconsistent override spam from webrtc, which is just annoying.
(In reply to Nathan Froyd [:froydnj] from comment #5)
> > @@ +2290,5 @@
> > > +            # something that MSVC currently warns about.
> > > +            CXXFLAGS="$CXXFLAGS -Wno-inconsistent-missing-override"
> > > +            # We declare |operator delete(void*)| differently than MSVC,
> > > +            # which causes a raft of warnings if we leave this enabled.
> > > +            CXXFLAGS="$CXXFLAGS -Wno-implicit-exception-spec-mismatch"
> > 
> > Can you provide more detail here, because this might actually be important.
> 
> The declaration from <new> says:
> 
> void __CRTDECL operator delete(void *) _THROW0();
> 
> and the warning says:
> 
> function previously declared with an explicit exception specification
> redeclared with an implicit exception specification
> 
> Hm.  I think that means our clang isn't turning off exceptions, because
> <xstddef> only defines THROW0 to something if _HAS_EXCEPTIONS.  Maybe that's
> not a good thing...

I don't completely understand this warning now, because we *are* passing -D_HAS_EXCEPTIONS=0, so there's no explicit exception specification on the function.  Unless clang is defining |operator delete| internally and then complaining that MSVC's headers (not ours) redeclare it improperly?  In which case I don't think there's anything we can do here, unless clang-cl supports -fno-exceptions?
https://hg.mozilla.org/mozilla-central/rev/3d2670d0cc5e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Sigh, apparently mistyped the bug number for a bustage fix.

Mike, WDYT about comment 5 and following?
Status: RESOLVED → REOPENED
Flags: needinfo?(mh+mozilla)
Resolution: FIXED → ---
(In reply to Nathan Froyd [:froydnj] from comment #7)
> I don't completely understand this warning now, because we *are* passing
> -D_HAS_EXCEPTIONS=0, so there's no explicit exception specification on the
> function.  Unless clang is defining |operator delete| internally and then
> complaining that MSVC's headers (not ours) redeclare it improperly?  In
> which case I don't think there's anything we can do here, unless clang-cl
> supports -fno-exceptions?

Could you check what a .i looks like for one of the files where the warning happens?
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #11)
> Could you check what a .i looks like for one of the files where the warning
> happens?

Sure.  delete is declared via <new> as:

|void __cdecl operator delete(void *) ;|

(None of the |delete| or |new| variants have a throwing declaration on them)
Flags: needinfo?(mh+mozilla)
Comment on attachment 8698586 [details] [diff] [review]
suppress numerous clang-style warnings when using clang-cl

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

Considering comment 12, let's go for it.
Attachment #8698586 - Flags: review+
Flags: needinfo?(mh+mozilla)
https://hg.mozilla.org/mozilla-central/rev/0912f477bd73
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: