[clang] -Wc++11-narrowing turned into error despite warning suppression (-w)
#92630
Comments
github-actions
bot
added
the
clang
EugeneZelenko
added
clang:diagnostics
|
It looks like We are treating it as ill-formed which is an error, not a warning. |
|
Perhaps our docs need to be updated? It SEEMS to me (though I haven't dug into it too much), that our warnings-as-default-errors (which are supposed to be 'things that are really errors, but we cant really make unignorable quite yet) are not suppressed with that FWIW, I think of |
|
I had discovered this a while back as well -- However, it does appear to be by design: @jyknight, you authored 3f8b916 -- was there a reason why your changes didn't include warnings that default to an error? |
|
I think this is working-as-intended. Clang's -w matches GCC's semantics: -w suppresses warnings but not I believe Clang has much more DefaultError diagnostics than GCC and therefore non-conforming code might feel Clang is "more noisy". The GCC patch description says:
In the future, we might see more GCC DefaultError diagnostics that cannot be suppressed with -w. |
|
This is non-sensical. Don't call something a warning if it's an error (by default). If however it is just a warning (e.g. the compiler will produce something highly probable to be useful), then
|
The big hammer no longer works with GCC 14's -Wnarrowing: I believe over time GCC will have more DefaultError diagnostics that can be suppressed with -Wno-xxx but not with -w. |
I disagree with your reasoning that this is by design -- we cannot point to GCC's behavior as rationale here. I'm still trying to dig up any information that suggests this behavior is by design in Clang and I've yet to find any. The fact that this issue keeps being raised by users as we add more default-error warnings suggests that we should strongly consider changing our behavior. Reopening this issue. |
I can actually see this both ways, but it depends what you think of when you see "warning-as-default-error". If you consider it: "A very serious warning, we've decided to make stop your build by default", I could see expecting If you consider it: "An error that we need folks to be able to skip sometimes, so we use the warning infrastructure to do this", I lean towards the latter, we typically do warning-as-default-error for things that we want to be a true error, but that there are a handful of usecases in the wild that we want to give time to change. From that perspective, I think it makes sense for |
|
I can squint to see it both ways. However, I'm not convinced it matters which way you squint at it. We should not require our users guess our intention as to whether a diagnostic is a serious warning or an unserious error. To me, the important thing is that these are warning flags -- we expose them with a I think |
I just think that is a shame to do in this case. Yes, it surprises users, but the point of warnings-as-errors (because we want to make them a real Effectively: Warnings-as-default-errors are special. They have an intrinsic meaning of "we are going to break you for real someday ". They are thus more serious than a normal warning, and should have special care taken around them. |
No, they don't. We have 56 diagnostics that are and so on. I took a look through the list and I would say there's not really a "most diagnostics do X" kind of grouping for them. There's a mixture of extensions and standards features, C and C++ features, ones we may want to turn into an error only someday and ones we may want to leave as downgradable, etc. |
I did a look through the list when I posted that above, and my impression was roughly 1/2 were "we would love to make these full errors someday...". That said, it is definitely a strategy we have used for cases where we wish we could make them errors, ,with the hope we could 'someday'. |
Definitely agreed! But again, our users should not have to make this distinction; that's an internal implementation detail. We expose these as warnings with a warning group and Also worth considering is that we have errors that users upgrade into warnings, and |
I'm concerned about the following situation: 1- Tons of users use 2- We have something we REALLY want to be an error, but the transition to doing so requires alerting our users of it so they can have time to fix it (perhaps because the 'fix' for it is non-trivial amounts of work). Historically we use warning-as-default-error here. 3- After said transition period, we make it a 'real' error. All the non Changing |
On the one hand, I agree with your concern. However, IMO, If the user specified |
I consider them to be somewhat different circumstances, but can see where you are coming from. I wouldn't be opposed to changing the Werror behavior, but I'm not strongly favored one way or the other.
If we are willing to take the messaging (and perhaps document it!) that I don't have a good feel of who uses |
|
I'm definitely in favor of the keeping the status-quo. I consider default-error diagnostics as "errors that we allow people to opt out of", not "warnings we happen to error on". Yes, we spell the opt-out using the same diagnostic framework as for warnings, because that's useful and convenient, but they are really a different category of issue, in my mind. Thus, I think it's entirely reasonable and defensible for I'll note that we do provide -Wno-everything if you really really really want NO diagnostics except hard-errors. I don't recommend anyone use that, but it's there, and it does provide this functionality... |
The status quo is inconsistent and I think that needs to change. I'm a strong -1 for keeping the status quo (broadly speaking); I want
I think it's equally as reasonable and defensible for it to mean "disable all warnings" and we don't make users differentiate between warnings upgraded to errors by the implementation and warnings upgraded to errors by the build script.
I agree that they're different categories to us as compiler authors. However, we are not typical users and that's important to keep in mind. Stepping back: the diagnostic the user gets for this code is: and given: the diagnostic the user gets for this code is: In both cases, we give a If something has to give (and I think it does because I really don't think the status quo is defensible in its inconsistency), then I think making |
I don't think this hypothetical scenario is giving users' ability to make an informed decision enough credit. That things can break if you ignore all warnings and update your compiler is such an obvious consequence that we should afford users the benefit of the doubt that this is intentional; IOW, the fact that this might happen does not invalidate the use cases for actually ignoring all warnings. Concrete use caseFollowing the example from the OP: Tensorflow documents their use of In this case, I really want the intent of the upstream developers to be honoured. The alternative is a failed build after several hours (TF is heavy...), find the "special warning", disable it, repeat. Needless to say this is a frustrating waste of time - I do not want to be "protected" from upstream's explicit choice, especially for something as (comparatively) benign as a narrowing conversion, which has a well-defined meaning even if it is lossy. It's the responsibility of the tensorflow developers to ensure their code does the correct thing (and they have multiple platforms/compilers that provide signals on this), and if indeed a new compiler version errors hard on a newly-diagnosed bug, it will be fixed upstream (or as a user, I stay on an older compiler for the time being which "works"). In either case, it's not appropriate for the compiler to second-guess the intent à la "I don't think you really meant all warnings 😉". |
I disagree with the assertion that this option is "inconsistent". I do not believe it is so. Perhaps it helps to think of the option as having two behaviors:
This is a useful set of behavior. It is not the only possible useful behavior, and I can see that other behaviors could also make sense, but it's the behavior GCC has long had, and that Clang has also now long had (though much less long than GCC). So I do not think that there is good justification to change that. In particular, I think it is also useful, and expected, that
Now, this seems a fruitful avenue in which to investigate improvements. I do agree that it is weird and confusing that default-error diagnostics simply print "-Wfoo" at the end, with no indication as to why they're being emitted as an error, despite "appearing" to be just a warning with no |
@AaronBallman Could you elaborate on that? The problem I encounter is that I need to pass different flags to different compilers to suppress a particular warning, for example:
GCC complains when passing an unknown flag (I suppose I could file a bug to them) and the build errors out, which is misleading to the user.
|
As far as the different warning flags: we attempt to keep our names the same when they do the same thing. In your example, note the clang group is ONLY for function pointer types (not all pointer types!), so are not the same thing. Typically this is something you solve in your cmake script. That said, we're definitely open to patches that name the same operation/checks the same thing. We also have the ability to put groups into other groups, so if we find 2 of our warning-groups that collectively do the same thing as 1 of GCCs, we can put them in the same group. That said, this ends up being a bit of a 'patches welcome'. Flags get developed in parallel to the point that we don't necessarily see what is happening on the GCC side when choosing a name (or vice versa). |
|
Thanks for the detailed explanation! |
I'm scratching my head at the following case, where we're trying to build the most recent tensorflow in conda-forge (tensorflow in itself is already a challenge to handle, but we have some distro-specificities on top that make for fun afternoons 😅), and running into the following on osx-64/osx-arm64 with clang 18.1.5 (also 16.0.6):
What's causing me surprise is that part of the invocation is
-w(next to the-std=c++17), which is documented to "Suppress all warnings". That should be the absolute antithesis to turning warnings into errors, so I really don't know how-Wc++11-narrowingends up failing this build (which also runs fine on linux with GCC despite tighter error settings than clang on osx)). I found a couple of old issues related to-Wc++11-narrowing, but nothing that stood out to me - apologies if this ends up being a duplicate.FWIW, tensorflow specifies is settings mainly here, though how flags get piped through bazel is not super-obvious (especially as the failure is happening in a vendored subfolder that itself has bazel build instructions).
PS. the compiler-wrapper being used does not touch the warning flags.
The text was updated successfully, but these errors were encountered: