aaron.ballman added a comment.
In D55628#1335841 <https://reviews.llvm.org/D55628#1335841>, @erik.pilkington
wrote:
> After looking through some users of `#pragma clang attribute`, I don't think
> that the begin/end solution is what we want here. Some users of this
> attribute write macros that can expand to different attributes depending on
> their arguments, for instance:
>
> AVAILABILITY_BEGIN(macos(10.12)) // expands to an availability attribute
> AVAILABILITY_BEGIN(ios(10))
> // some code...
> AVAILABILITY_END
> AVAILABILITY_END
>
>
> This code makes sense and is safe, but in this case rewriting
> AVAILABILITY_BEGIN to use a hypothetical `pragma clang attribute begin`/`end`
> would be a breaking change, which isn't acceptable.
Good catch!
> So I think that we want stack semantics, but scoped within the
> `AVAILABILITY_BEGIN` macro's namespace. That way, we can support multiple
> `push`es in the same macro, without having having different macros
> accidentally pop each other.
>
> As for a syntax for this, I chose the following (basically, @dexonsmith's
> idea with a '.'):
>
> #pragma clang attribute NoReturn.push (__attribute__((noreturn)),
> apply_to=function)
> int foo();
> #pragma clang attribute NoReturn.pop
>
>
> I think the '.' makes the nested relationship (i.e. the push is contained
> within the namespace) more clear to C programmers, and hopefully clears up
> the confusion. @AaronBallman: WDYT?
This is definitely a novel syntax, but I like how clear it is that the label
acts as the scope being pushed and popped. One question I have is with
interactions from unlabeled pushes and pops:
#pragma clang attribute MyNamespace.push (__attribute__((annotate)),
apply_to=function)
#pragma clang attribute push (__attribute__((annotate)), apply_to=function)
int some_func1();
#pragma clang attribute pop // does this pop the unlabeled push, or is it an
error?
#pragma clang attribute MyNamespace.pop
Or, similarly:
#pragma clang attribute push (__attribute__((annotate)), apply_to=function)
#pragma clang attribute MyNamespace.push (__attribute__((annotate)),
apply_to=function)
int some_func2();
#pragma clang attribute pop // does this pop the unlabeled push, or is it an
error?
#pragma clang attribute MyNamespace.pop
I suspect the answer is that it pops the unlabeled push and is not an error,
but I'd like to see a test case with it.
================
Comment at: clang/docs/LanguageExtensions.rst:2700
+Because multiple push directives can be nested, if you're writing a macro that
+expands to ``_Pragma("clang attribute")`` it's good hygiene (though not
----------------
Somewhere around here you should probably mention that an unlabeled pop does
*not* act like a "pop any namespace" action but instead acts as a "pop the last
unlabeled namespace" action. Some users may find that behavior surprising
otherwise.
================
Comment at: clang/docs/LanguageExtensions.rst:2709-2714
+ #define ASSUME_NORETURN_BEGIN _Pragma("clang attribute AssumeNoreturn.push
([[noreturn]], apply_to = function)
+ #define ASSUME_NORETURN_END _Pragma("clang attribute AssumeNoreturn.pop")
+
+ ASSUME_NORETURN_BEGIN
+ void function(); // function has [[noreturn]]
+ ASSUME_NORETURN_END
----------------
I think it would be useful for the example to show the problematic situation
this feature is meant to avoid.
================
Comment at: clang/lib/Parse/ParsePragma.cpp:3161
+ if (!Tok.is(tok::period)) {
+ PP.Diag(Tok.getLocation(), diag::err_pragma_attribute_expected_period)
+ << II;
----------------
Can you reuse `diag::err_expected_after` instead of making a new diagnostic?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55628/new/
https://reviews.llvm.org/D55628
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
