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}} \
+ // [email protected]:* {{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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
