[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

This revision was automatically updated to reflect the committed changes. Closed by commit rGb36bdfe5ca0c: [cmake] Centralize LLVM_ENABLE_WARNINGS option (authored by kastiglione). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87243/new/

[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

compnerd accepted this revision. compnerd added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87243/new/ https://reviews.llvm.org/D87243

[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

kastiglione added a comment. for context this caused standalone swift-lldb builds to have warnings disabled via `-w` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87243/new/ https://reviews.llvm.org/D87243

[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

kastiglione added a comment. @compnerd Saleem, what do you think? (see also my reply to you) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87243/new/ https://reviews.llvm.org/D87243 ___ cfe-commits

[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

JDevlieghere accepted this revision. JDevlieghere added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87243/new/ https://reviews.llvm.org/D87243 ___ cfe-commits mailing list

[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

kastiglione updated this revision to Diff 290597. kastiglione edited the summary of this revision. kastiglione added a comment. Add LLVM_ENABLE_WARNINGS to LLVMConfig.cmake.in Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87243/new/

[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

JDevlieghere added a comment. In D87243#2261687 , @kastiglione wrote: > If an LLVM install disabled `LLVM_ENABLE_WARNINGS`, should other builds > inherit that? I would think no, but is there a precedent for that that to be > the case? Yes, most of the

[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

kastiglione added a comment. If another project defines `LLVM_ENABLE_WARNINGS` before loading `HandleLLVMOptions`, it seems correct to me that the first one is used. This change ensures the default value of ON is setup at the last possible opportunity, before `LLVM_ENABLE_WARNINGS` is read and

[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

compnerd added a comment. The change itself is fine, but what about downstream projects which define the option? Will that trigger a warning? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87243/new/ https://reviews.llvm.org/D87243

[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

kastiglione added a comment. If an LLVM install disabled `LLVM_ENABLE_WARNINGS`, should other builds inherit that? I would think no, but is there's a precedent for that that to be the case? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87243/new/

[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

JDevlieghere added a comment. You need to add `LLVM_ENABLE_WARNINGS` to `LLVMConfig.cmake.in` so that the standalone builds know what value was set in the LLVM build. I think with the current patch the other projects won't inherit the value and just default to `ON`? Repository: rG LLVM

[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

DavidTruby accepted this revision. DavidTruby added a comment. LGTM and seems to work from the flang side. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87243/new/ https://reviews.llvm.org/D87243 ___

[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

kastiglione added a comment. The `LLVM_ENABLE_WARNINGS` variable is read only within `HandleLLVMOptions.cmake`. Outside declarations/defaults have effect only when `HandleLLVMOptions` is loaded, one way or another. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

kastiglione added a comment. @lebedev.ri `clang/CMakeLists.txt` contains `include(HandleLLVMOptions)` for its standalone build, I think this covers the issue you're pointing out Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87243/new/

[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

lebedev.ri added a comment. This is not nessesairy correct, at least the clang can be built in standalone mode, not as part of monorepo checkout (i.e. it's `clang/cmakelists.txt` is the root cmakelists, not `llvm/cmakelists.txt`'s) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D87243: [cmake] Centralize LLVM_ENABLE_WARNINGS option

kastiglione created this revision. Herald added subscribers: llvm-commits, libcxx-commits, cfe-commits, mgorny. Herald added a reviewer: DavidTruby. Herald added projects: clang, libunwind, LLVM. Herald added a reviewer: libunwind. kastiglione requested review of this revision. Repository: rG