[PATCH] D119707: [C2x] Implement WG14 N2764 the [[noreturn]] attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Headers/stdnoreturn.h:19
+/* The noreturn macro is deprecated in C2x. */
+#pragma clang deprecated(noreturn)
+
----------------
aaron.ballman wrote:
> jyknight wrote:
> > Is this actually useful? Isn't it sufficient to have the include-time warning for this header?
> I think it gives a more precise diagnostic than the header inclusion one, and it will diagnose closer to where the use happens instead of at the include. Might not be the most useful, but it I think there's some utility.
Okay, I've convinced myself to remove this a well for the same reason we don't want to warn on `[[noreturn]]` which expands to `[[_Noreturn]]` -- the issue isn't with use of the attribute, it's with use of the header file.  I don't want to risk a user writing `[[noreturn]]` and hearing that the `noreturn` macro is deprecated and they think that means that `[[noreturn]]` is deprecated.


================
Comment at: clang/lib/Headers/stdnoreturn.h:22
+/* Including the header file in C2x is also deprecated. */
+#warning "the '<stdnoreturn.h>' header is deprecated in C2x"
+#endif
----------------
jyknight wrote:
> aaron.ballman wrote:
> > jyknight wrote:
> > > Would be nice to mention the solution, as well. E.g.
> > > 
> > > "The '<stdnoreturn.h>' header has been deprecated in C2x: including it no longer necessary in order to use 'noreturn'. "
> > I can add that, but it is necessary in order to use 'noreturn' as a function specifier, so that wording isn't quite right. e.g., `noreturn void func(void);` is valid C today if you `#include <stdnoreturn.h>`
> Ah, I missed that subtlety. Maybe say:
> 
> "The '<stdnoreturn.h>' header has been deprecated in C2x; either use the _Noreturn keyword or the [[noreturn]] attribute."
> 
Sure, I can go with that.


================
Comment at: clang/test/Sema/c2x-noreturn.c:41-43
+[[noreturn]] void func6(void); // all-warning {{the '[[_Noreturn]]' attribute spelling is deprecated in C2x; use '[[noreturn]]' instead}} \
+                               // c2x-warning {{macro 'noreturn' has been marked as deprecated}} \
+                               // c2x-note at stdnoreturn.h:* {{macro marked 'deprecated' here}}
----------------
jyknight wrote:
> aaron.ballman wrote:
> > jyknight wrote:
> > > If you've written [[noreturn]] in your code, you're doing the right thing already. Why bother emitting a warning? The problem that should be corrected is at the point of inclusion of stdnoreturn.h, not the spelling here.
> > > 
> > > I'd suggest only warning about "[[_Noreturn]]" if the user actually //wrote// it like that, and not when it's expanded from the "noreturn" macro.
> > > I'd suggest only warning about "[[_Noreturn]]" if the user actually wrote it like that, and not when it's expanded from the "noreturn" macro.
> > 
> > I'm not keen on that design. The goal of this diagnostic is to let people know that they have a surprise in their code (that this macro is expanding to something they may not expect) at the location the surprise happens. Consider:
> > 
> > ```
> > // TU.c
> > #include <stdnoreturn.h>
> > #include "my_header.h"
> > 
> > // my_header.h
> > [[noreturn]] void func(void);
> > ```
> > my_header.h doesn't see the inclusion of stdnoreturn.h, so finding out about the macro may inform the developer to be more protective of using the attribute.
> But that's exactly my point. There _is_ no surprise in this code. It works exactly as expected. 
> The my_header.h code is using the new non-deprecated spelling, and is getting the correct and desired behavior -- even though someone else has included stdnoreturn.h. That it's working properly due to there existing a compatibility attribute "_Noreturn" is basically an irrelevant detail to any user.
> 
> Emitting any warnings for this line of code seems potentially even harmful. Consider: what should my_header.h be doing differently due to the warning? Nothing. Ideally, they should change nothing. Yet, if we do emit warnings for this usage, it'll likely cause some people to try to add push/undef/pop gunk to their headers to avoid the warning, which makes the code worse than if they did nothing.
> 
Okay, thank you for sticking with me, that logic makes sense to me. I'll remove the diagnostic in this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119707