[PATCH] D99825: [cmake] Enable -Werror=return-type for clang

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

>> The two I see are `date-time` and `unguarded-availability-new`
>
> There's also `non-virtual-dtor` and `suggest-override`.

I think those are only turned into errors for the feature test (the compiler's being run during the cmake configuration process with a sample that /should/ produce a specific warning/error (or shouldn't produce a specific error, if the test is validating a certain false positive case is not diagnosed) - it's easier for the script to test for non-zero exit code (& so uses -Werror) than parsing the diagnostic output.

>> I'd rather not get into the territory of classifying different warnings - and encourage people to turn on -Werror generally, as many buildbots do
>
> I agree it would be good if `-Werror` were used pretty much everywhere. But with it not being used everywhere, downstream projects have warnings and can't flip the switch.

Not sure I follow this - presumably downstream users wouldn't be using LLVM's warning or werror flags, and would configure their own?

> A change like this is a small incremental improvement that prevents a certain kind of error at compile time, like the other `-Werror=...` flags. I think there's an argument to be made that this warning is more severe than some of the others marked as errors.

I mean, there are lots of other warnings that represent pretty severe results - `-Wnull-dereference`, `-Warray-bounds` ?

> And again some projects in llvm already turn it on, so there is evidence supporting it as a good thing.
>
> Unfortunately I don't know what you mean by "I'd rather not get into the territory". Will this have a cost to you? Or do you mean you wish to preemptively prevent debates about which warnings should/shouldn't be errors?

Yep, pretty much that (don't want to start trying to classify some warnings as more severe than others, etc - generally I'm all for treating all warnings on a self-hosted clang build as errors and we either fix clang if the warning is bad or fix the code if the warning is good)

> Did you have any comments on @xbolva00's comments? I thought those were pretty compelling.

There are many other warnings that diagnose UB the compiler can do bad things to - so I don't agree it's a good justification for this particular warning.

@aaron.ballman's point that maybe this should be -Werror by default in clang sounds plausible - because not all projects will be using -Werror, etc. If that helps some users who work on LLVM itself but don't want to turn on -Werror (not sure why - the buildbots will do it and so things will break/you'll have to fix stuff if you're not building with -Werror using a fairly recent clang) that's OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99825