<div dir="ltr">I'd either keep the warning on or turn it off, not just turn it into a warning rather than an error.<br><br>Is it impractical to cleanup the existing cases? (though I'm not opposed to Justin's suggestion of grandfathering in projects that violate this - but nicer to avoid it if it's not too much hassle to cleanup)</div><br><div class="gmail_quote"><div dir="ltr">On Tue, Jun 13, 2017 at 4:40 PM Justin Bogner <<a href="mailto:mail@justinbogner.com">mail@justinbogner.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Roman Lebedev via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> writes:<br>
> lebedev.ri created this revision.<br>
> Herald added a subscriber: mgorny.<br>
><br>
> Diagnostic `-Wcast-qual` is not enabled by default in gcc/clang, It<br>
> was enabled explicitly some time ago.<br>
> Later, clang has learned to do `-Wcast-qual`, like gcc does.<br>
> While that clang diagnostic is compatible with gcc's, it additionally<br>
> catches one more case, see <a href="https://reviews.llvm.org/D34153" rel="noreferrer" target="_blank">https://reviews.llvm.org/D34153</a>.<br>
> And last but not least, clang's diagnostic `-Wcast-qual` only works<br>
> for `C` code (sic!)<br>
> In <a href="https://reviews.llvm.org/D33102" rel="noreferrer" target="_blank">https://reviews.llvm.org/D33102</a>, i have extended it to work for<br>
> `C++` code too.<br>
> Once that landed, `-Werror` buildbots broke. That is when i was made<br>
> aware that this diagnostic is enabled in llvm builds.<br>
> I have fixed that particular buildbot-discovered failure in<br>
> <a href="https://reviews.llvm.org/D34153" rel="noreferrer" target="_blank">https://reviews.llvm.org/D34153</a>.<br>
> However, when i tried building clang stage2 with self-built clang<br>
> stage1 (with <a href="https://reviews.llvm.org/D33102" rel="noreferrer" target="_blank">https://reviews.llvm.org/D33102</a>), i have quickly<br>
> discovered that some sub-projects also exhibit this warning.<br>
> Specifically, **openmp** and **libcxx** subprojects produce **401**<br>
> -Wcast-qual warnings, and i //believe// not all of these warnings are<br>
> clang-specific.<br>
> (i don't think i have all subprojects checked-out, so there may be even more)<br>
> This tells two things:<br>
><br>
> - these subprojects are not being built by `-Werror` buildbots. (at<br>
> least as part of LLVM, not standalone)<br>
> - no one builds LLVM by GCC with these subprojects with `-Werror` enabled.<br>
><br>
> So, if <a href="https://reviews.llvm.org/D33102" rel="noreferrer" target="_blank">https://reviews.llvm.org/D33102</a> is to land, this //may// not<br>
> break `-Werror` buildbots. This is bad and good.<br>
> But. If someone wants to build LLVM with these subprojects and with<br>
> `-Werror`, he/she/it will face a lot of fatal warnings.<br>
> It is unfeasible for me to fix all these warnings before re-landing<br>
> <a href="https://reviews.llvm.org/D33102" rel="noreferrer" target="_blank">https://reviews.llvm.org/D33102</a>, thus i propose this diff.<br>
><br>
> After <a href="https://reviews.llvm.org/D33102" rel="noreferrer" target="_blank">https://reviews.llvm.org/D33102</a> has landed, the buildbots should<br>
> //probably// be adjusted to catch these cases, and i will see if i can<br>
> help fix these issues and revert this patch.<br>
><br>
> Thoughts?<br>
<br>
I'd rather we only set this to -Wno-error for the projects that actually<br>
have this warning, and we leave it as is for the projects that are<br>
already clean.<br>
<br>
> Repository:<br>
>   rL LLVM<br>
><br>
> <a href="https://reviews.llvm.org/D34176" rel="noreferrer" target="_blank">https://reviews.llvm.org/D34176</a><br>
><br>
> Files:<br>
>   cmake/modules/HandleLLVMOptions.cmake<br>
><br>
> Index: cmake/modules/HandleLLVMOptions.cmake<br>
> ===================================================================<br>
> --- cmake/modules/HandleLLVMOptions.cmake<br>
> +++ cmake/modules/HandleLLVMOptions.cmake<br>
> @@ -521,6 +521,7 @@<br>
>  if (LLVM_ENABLE_WARNINGS AND (LLVM_COMPILER_IS_GCC_COMPATIBLE OR CLANG_CL))<br>
>    append("-Wall -W -Wno-unused-parameter -Wwrite-strings" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)<br>
>    append("-Wcast-qual" CMAKE_CXX_FLAGS)<br>
> +  append("-Wno-error=cast-qual" CMAKE_CXX_FLAGS)<br>
><br>
>    # Turn off missing field initializer warnings for gcc to avoid noise from<br>
>    # false positives with empty {}. Turn them on otherwise (they're off by<br>
<br>
</blockquote></div>