[PATCH] D55628: Add support for "labels" on push/pop directives in #pragma clang attribute

[Aaron Ballman via Phabricator via cfe-commits]

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