I agree with https://easyaspi314.github.io/gcc-vs-clang.html?fbclid=IwAR1VA0qxiWVUusOQUv5z7JESS7ZpeJy-UqAI5mnJscofGLqXcqeErIUB2gU, current warning message is not very good. We should try to improve it..
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I think the new text is more clear than the old text, so I'm fine with the current direction you're going.
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
585–588 | Should we change this wording as well? Along with any other instances of similar wording (I don't recall if we have something similar for lambdas or if we defer to the function diagnostics in that case)? |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
585–588 | +1, I agree. It would be better if somebody more familiar with Obj-C / coroutines and related terminology would improve these text. |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
585–588 | I think you've already got the gist of it. block does not return a value; non-void blocks must return a value, and similar for coroutines. In fact, I wonder why we don't just combine these diagnostics for all three situations and use a %select for them? That seems like a reasonable thing to do (having not put a ton of thought into it). |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
580 | As long as we're messing with this wording: Does it actually help any human reader to distinguish "control paths" versus simply "paths"? And could we DRY it up by saying
I don't think the Coroutines warning needs to specifically call out "undefined behavior," unless it is trying to say that the code is IFNDR. Of course falling off the end of a function is UB if it ever actually happens at runtime; that's no different whether it's a coroutine or a regular function/block. The only reason for a wording difference in the Coroutines case is that the colloquial notion of a "(non-)void coroutine" (whose return type would be something like task<void>) is slightly less familiar than the colloquial notion of a "(non-)void function" (whose return type is literally void). |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
580 | I am also wondering if we could use some better term than "control paths", but I haven't found any :) Paths sound to general for me but I am not strongly opposed. if @aaron.ballman is happy with your wording, so do I. |
I am not sure if "may" is ideal, it could suggest that compiler is not very sure and emits just some conservative message.
"not all paths in this non-void {function,block} return a value" gives some extra confidence that compiler knows there is a buggy control path.
I'd probably go with "non-void %select{function|block|coroutine}0 does not return a value %select{|in all control paths}1"
We cannot use it for block; it is a hard error. Corountine and function can be merged as suggested.
Looking at Sema/AnalysisBasedWarnings.cpp, reworking it to "select" would take some time so I will prepare just simple patch without "select".
We can use .Text to share diagnostic text between warnings and errors (we do this elsewhere), but I don't insist for this patch.
LGTM aside from a commenting request.
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
579 | A FIXME comment that we'd like to combine these diagnostics eventually would be appreciated. | |
6861 | And maybe move these two up closer to the other duplicate-ish diagnostics. |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
6861 | This is in so I am not sure if should really move it. |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
6861 | I hadn't noticed that, let's leave it here. Good catch! |
A FIXME comment that we'd like to combine these diagnostics eventually would be appreciated.