Closed Bug 1476475 Opened 2 years ago Closed 2 years ago

Disable c++98-compat warning for clang-cl

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

There are lots of this warnings. It is annoying, and I don't believe it makes sense for us. We are not going back to C++98, are we?
Assignee: nobody → xidorn+moz
Attachment #8992810 - Flags: review?(core-build-config-reviews) → review?(nfroyd)
Comment on attachment 8992810 [details]
Bug 1476475 - Disable c++98-compat warnings for clang-cl.

https://reviewboard.mozilla.org/r/257646/#review264554

Thanks!

::: old-configure.in:1033
(Diff revision 1)
> +            # We don't worry about backward-compat with C++98.
> +            CXXFLAGS="$CXXFLAGS -Wno-c++98-compat -Wno-c++98-compat-pedantic"

Please add the same stanza to js/src/old-configure.in as well.
Attachment #8992810 - Flags: review?(nfroyd) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d752b1eb89fe
Disable c++98-compat warnings for clang-cl. r=froydnj
I am pretty sure most (all?) of these are coming from pkix enabling -Wall. I would have preferred to see bug 1476486 land and then see if these c++98 things still remain. Enlarging our top-level command line for this may be overkill.
You are probably right. I saw that in other files in-tree, but that's because they are referenced in the directories with -Wall. There is another source, though, which is certverifier. We should disable c++98 thing from there as well.
Yeah, in bug 1090497 I found that some huge fraction of warnings are coming from pkix and certverifier (plus the pragma pack in bug 1472254).
Depends on: 1476486
security/pkix/warnings.mozbuild has -Wno-c++98-compat when building with ... clang. but not when building with clang-cl.
(In reply to Mike Hommey [:glandium] from comment #8)
> security/pkix/warnings.mozbuild has -Wno-c++98-compat when building with ...
> clang. but not when building with clang-cl.

That's what I want to address in bug 1476486.
Attachment #8992810 - Attachment is obsolete: true
Comment on attachment 8993581 [details]
Bug 1476475 - Use -W4 for certverifier with clang-cl.

https://reviewboard.mozilla.org/r/258286/#review265352

::: commit-message-65c4b:1
(Diff revision 1)
> +Bug 1476475 - Add a bunch of warning suppression to certverifier. r?Build

I think you should either:
1. avoid using -Wall in clang-cl because clang-cl will translate -Wall into clang -Weverything.
   -W4 (which will be translated into -Wall -Wextra) would be appropreate here. In guess you don't have to fix bug 1477161, 1477172, and 1477173 in this case.
2. Or, use -Weverything also in clang (non-cl). Why do you use different warning levels between clang-cl and clang?
OK, I guess I would use -W4 instead...
Thanks for that suggestion. Using -W4 significantly reduces the number of warnings!
Attachment #8993581 - Flags: review?(core-build-config-reviews) → review?(nfroyd)
Comment on attachment 8993581 [details]
Bug 1476475 - Use -W4 for certverifier with clang-cl.

https://reviewboard.mozilla.org/r/258286/#review265422

::: security/certverifier/moz.build:65
(Diff revision 2)
>  TEST_DIRS += [
>      'tests/gtest',
>  ]
>  
> +if CONFIG['CC_TYPE'] == 'clang-cl':
> +    # -W4 of clang-cl is mapped to clang's -Wall -Wextra

Maybe note that clang-cl's `-Wall` maps to `-Weverything`, which turns on way too much, so we're using `-W4` instead?
Attachment #8993581 - Flags: review?(nfroyd) → review+
Comment on attachment 8993581 [details]
Bug 1476475 - Use -W4 for certverifier with clang-cl.

https://reviewboard.mozilla.org/r/258286/#review265540

::: security/certverifier/moz.build:70
(Diff revision 2)
> +    # -W4 of clang-cl is mapped to clang's -Wall -Wextra
> +    CXXFLAGS += ['-W4']
> +else:
> -CXXFLAGS += ['-Wall']
> +    CXXFLAGS += ['-Wall']
> +
>  if CONFIG['CC_TYPE'] in ('msvc', 'clang-cl'):

Does this still need to include 'clang-cl'?
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ffb7bfbfc328
Use -W4 for certverifier with clang-cl. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/ffb7bfbfc328
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Huge reduction in compiler warnings:


== Change summary for alert #14569 (as of Wed, 25 Jul 2018 08:56:17 GMT) ==

Improvements:

 84%  compiler warnings windows2012-64 asan debug     6,856.96 -> 1,070.50
 84%  compiler warnings windows2012-32-noopt debug    6,823.96 -> 1,065.50
 84%  compiler warnings windows2012-32 debug          6,823.96 -> 1,065.50
 84%  compiler warnings windows2012-64 debug plain    6,885.96 -> 1,095.50
 84%  compiler warnings windows2012-64-noopt debug    6,887.96 -> 1,097.50
 84%  compiler warnings windows2012-64 debug          6,887.96 -> 1,097.50
 82%  compiler warnings windows2012-32 opt rusttests  5,850.96 -> 1,073.50
 82%  compiler warnings windows2012-32 pgo            5,850.96 -> 1,073.50
 82%  compiler warnings windows2012-32 opt            5,850.96 -> 1,073.50
 82%  compiler warnings windows2012-64 asan opt       5,867.96 -> 1,079.42
 81%  compiler warnings windows2012-64 opt plain      5,897.96 -> 1,105.42
 81%  compiler warnings windows2012-64 opt rusttests  5,900.29 -> 1,105.75
 81%  compiler warnings windows2012-64 opt            5,897.96 -> 1,105.50
 81%  compiler warnings windows2012-64 pgo            5,897.96 -> 1,105.58

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14569
You need to log in before you can comment on or make changes to this bug.