This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [ci] Enable LIBCXX_ENABLE_WERROR where possible
ClosedPublic

Authored by mstorsjo on Feb 11 2022, 12:09 PM.

Details

Summary

Only opt out from it in the few configs where there still are build
warnings.

Diff Detail

Event Timeline

mstorsjo created this revision.Feb 11 2022, 12:09 PM
mstorsjo requested review of this revision.Feb 11 2022, 12:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2022, 12:09 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Also cc @philnik, regarding silencing GCC warnings.

I think you should put the -DLIBCXX_ENABLE_WERROR=NO in the cmake cache files. Why isn't there a cmake cache file for generic-gcc @ldionne?

I think you should put the -DLIBCXX_ENABLE_WERROR=NO in the cmake cache files. Why isn't there a cmake cache file for generic-gcc @ldionne?

There’s probably none, because no specific configuration is needed.

I don’t think we should involve the cmake cache files regarding LIBCXX_ENABLE_WERROR overall. Also, I think a command line argument (when enabled in run-buildbot) overrides what’s set in a cache file.

And I don’t think we should be enabling that option anywhere in the cache files. When building libcxx with the intent of *using* the library, we don’t want LIBCXX_ENABLE_WERROR enabled, because if it builds successfully, it’s probably fine to use even if it produced warnings. Enabling it should be strictly a CI thing (and when developing the library , if such a setup is wanted).

mstorsjo updated this revision to Diff 408060.Feb 11 2022, 2:47 PM

Rebase after fixing a missed instance of no_unique_address.

I think you should put the -DLIBCXX_ENABLE_WERROR=NO in the cmake cache files.

I prefer not to for the reasons @mstorsjo mentioned. I love using -Werror, but I feel it should always be an opt-in.
I want to see the CI green before approving.

I think you should put the -DLIBCXX_ENABLE_WERROR=NO in the cmake cache files.

I prefer not to for the reasons @mstorsjo mentioned. I love using -Werror, but I feel it should always be an opt-in.
I want to see the CI green before approving.

Yep, I noticed that there were some more warnings in the Ryu code. (I had tested building with clang-cl for aarch64, where those warnings weren't present.) I'll soon post a patch that should silence them.

Mordante accepted this revision as: Mordante.Feb 14 2022, 9:11 AM

LGTM, provided D119647 fixes the CI failures.

ldionne accepted this revision.Feb 14 2022, 10:01 AM
This revision is now accepted and ready to land.Feb 14 2022, 10:01 AM
mstorsjo updated this revision to Diff 408549.Feb 14 2022, 12:13 PM

Rebased on top of latest git main, now the clang-cl configs should be green.

This revision was automatically updated to reflect the committed changes.