[libcxx-commits] [PATCH] D107835: [libunwind] Compile with -Wunused-but-set-variable

Tue Aug 31 11:07:06 PDT 2021

MaskRay added a comment.

In D107835#2975100 <https://reviews.llvm.org/D107835#2975100>, @dblaikie wrote:

> In D107835#2975019 <https://reviews.llvm.org/D107835#2975019>, @MaskRay wrote:
>
>> Looks like the libunwind warning list is stronger than llvm's (scattered in `llvm/cmake/modules/HandleLLVMOptions.cmake`).
>> I think it is probably fine to not duplicate options which are also set in llvm/.
>
> I don't mind setting them if someone wants standalone builds to have more parity with non-standalone. But I'd object more to adding extra warnings per project without a pretty significant reason that one subproject should differ from others.

Agree.

For `-Wunused-but-set-variable` in libunwind, there is probably another justification: libunwind uses `-Wunused-parameter` while `llvm/cmake/modules/HandleLLVMOptions.cmake` disables it.
Since libunwind is `-Wunused-parameter` clean, we don't want to regress it.
-Wunused-but-set-variable is a close option in the family (though not under the -Wunused umbrella).

>> Perhaps not many groups use standalone builds, so losing some warning options is fine.
>>
>> llvm/'s warnings are under `LLVM_ENABLE_WARNINGS`, while libunwind specifics are unconditional. Now I am unsure what to do.
>
> If this flag isn't turned on for non-standalone builds/turned on in LLVM, then I'm not sure what issue the patch is intending to address. There wouldn't be any inconsistency, there wouldn't be any regression issue (like some builds checking the warning and some not checking it - so someone in the non-checking build committing a warning violation, breaking the checking build) if the flag wasn't added?
>
> But if someone wants to turn it on for the non-standalone build, that sounds OK. It's probably not especially painful to cleanup.

I support that `llvm/cmake/modules/HandleLLVMOptions.cmake` adds `-Wunused-but-set-variable` as well, after some cleanup.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107835/new/

https://reviews.llvm.org/D107835