dexonsmith added inline comments.
================
Comment at: clang/test/Sema/pragma-attribute-label.c:7
+
+#pragma clang attribute pop // expected-error{{'#pragma clang attribute pop'
with no matching '#pragma clang attribute push'}}
+#pragma clang attribute pop NOT_MY_LABEL // expected-error{{'#pragma clang
attribute pop NOT_MY_LABEL' with no matching '#pragma clang attribute push
NOT_MY_LABEL'}}
----------------
erik.pilkington wrote:
> aaron.ballman wrote:
> > dexonsmith wrote:
> > > aaron.ballman wrote:
> > > > Should we really treat this as an error? It seems to me that this
> > > > should be a warning because pop without a label could be viewed as "I
> > > > don't care what I'm popping, just pop it". Still worth warning about,
> > > > but maybe not worth stopping a build over.
> > > IMO this is most likely to be an implementation error on the part of a
> > > macro author, where the END macro is missing the label used in BEGIN.
> > > This makes the macro pair unsafe to mix with other macros. If the macro
> > > author doesn’t want safety, why use a label in the BEGIN macro at all?
> > >
> > > I see you’re envisioning this being used directly by an end-user, which I
> > > suppose is plausible, but I think the same logic applies. Why add a
> > > label to push if you don’t want to be precise about pop?
> > > Why add a label to push if you don’t want to be precise about pop?
> >
> > Why is this important enough to fail everyone's build over it as opposed to
> > warning users that they've done something that could be a bad code smell
> > and let -Werror usage decide whether to fail the build or not? It seems
> > like an extreme measure for something that has explicable "fallback"
> > behavior.
> My implicit assumption (which I should have been more clear about!) was that
> you'd only really ever write a label on a `push` in a BEGIN/END macro. In
> that world, you'd only ever see this case if 1) you're interacting with
> another macro that doesn't use the label convention, or 2) if you're
> interacting with manual push/pop code. In both of those cases, you'd end up
> popping the wrong attribute group and start applying attributes onto
> declarations that the programmer didn't intend.
>
> I'm fine with downgrading this to a warning, but IMO an error seems more
> appropriate. If we wanted to force 1) or 2) through the compiler then we'd
> also need to downgrade `pop UNPUSHED_LABEL` to a warning, which doesn't seem
> like the end of the world either.
I'm not fine with this just being a warning. The entire point is that the
stacks are independent; having them suddenly mixed here would be super
confusing. @aaron.ballman, please see my other comment for a longer
explanation.
================
Comment at: clang/test/Sema/pragma-attribute-label.c:15
+// Out of order!
+#pragma clang attribute pop MY_LABEL
+
----------------
erik.pilkington wrote:
> aaron.ballman wrote:
> > dexonsmith wrote:
> > > aaron.ballman wrote:
> > > > I feel like this should be diagnosed, perhaps even as an error. The
> > > > user provided labels but then got the push and pop order wrong when
> > > > explicitly saying what to pop. That seems more likely to be a logic
> > > > error on the user's part.
> > > On the contrary, the user is using two differently-named and independent
> > > macro pairs (A_BEGIN/A_END vs B_BEGIN/B_END) and has no idea they are
> > > implemented with _Pragma(“clang attribute ...”) under the hood. The
> > > point is to give the same result as two independent pragma pairs, whose
> > > regions do not need to be nested.
> > > On the contrary, the user is using two differently-named and independent
> > > macro pairs (A_BEGIN/A_END vs B_BEGIN/B_END)
> >
> > I don't think this is a safe assumption to make, and in this case, is
> > false. There are no macros anywhere in this test case.
> >
> > > The point is to give the same result as two independent pragma pairs,
> > > whose regions do not need to be nested.
> >
> > I don't find this to be intuitive behavior. These are stack manipulations
> > with the names push and pop -- pretending that they're overlapping rather
> > than a stack in the presence of labels is confusing. If I saw the code from
> > this test case during a code review, I would flag it as being incorrect
> > because the labels do not match -- I don't think I'd be the only one.
> I think the labels only really makes sense if you're writing macros that hide
> the pragma attribute stack (like ASSUME_X_BEGIN/END, for instance), which for
> better or for worse people do write, and in fact was the intended use case
> for #pragma clang attribute. I think if we were to write this feature again,
> we'd forgo the stack entirly and require every `push` to have a label and be
> in its own namespace. But this is the best we can do now.
>
> I don't really think that anyone should write a push label outside of a macro
> definition, since I agree that the semantics are a bit surprising when you're
> writing the #pragmas yourself without macros. I'll update this test case and
> the documentation to stress this point more. If you think this is going to be
> a potential pain point, maybe we can even warn on using a label outside of a
> macro definition.
>> The point is to give the same result as two independent pragma pairs, whose
>> regions do not need to be nested.
> I don't find this to be intuitive behavior. These are stack manipulations
> with the names push and pop -- pretending that they're overlapping rather
> than a stack in the presence of labels is confusing. If I saw the code from
> this test case during a code review, I would flag it as being incorrect
> because the labels do not match -- I don't think I'd be the only one.
As Erik says, the intention is that these are only used by macros.
Stepping back, the goal of this pragma (which we added in
https://reviews.llvm.org/D30009) is to avoid adding a new region-based pragma
every time we want to apply an attribute within a region. Clang has a lot of
these pragmas, in order to support independent macros like this:
```
#define A_BEGIN _Pragma("clang a push")
#define A_END _Pragma("clang a pop")
#define B_BEGIN _Pragma("clang b push")
#define B_END _Pragma("clang b pop")
#define C_BEGIN _Pragma("clang c push")
#define C_END _Pragma("clang c pop")
```
@arphaman came up with the idea of introduce "one pragma to rule them all",
`pragma clang attribute`, so that we didn't need to introduce a new pragma each
time we wanted an independent region:
```
#define A_BEGIN _Pragma("clang attribute push(a)")
#define A_END _Pragma("clang attribute pop")
#define B_BEGIN _Pragma("clang attribute push(b)")
#define B_END _Pragma("clang attribute pop")
#define C_BEGIN _Pragma("clang attribute push(c)")
#define C_END _Pragma("clang attribute pop")
```
However, we've realized that there is a major design flaw: these macros are not
independent, because they're using the same stack. @erik.pilkington has come
up with this solution using identifiers to namespace the attribute stacks,
allowing the macros to be independent (like they were before, when each had a
dedicated pragma):
```
#define A_BEGIN _Pragma("clang attribute push A (a)")
#define A_END _Pragma("clang attribute pop A")
#define B_BEGIN _Pragma("clang attribute push B (b)")
#define B_END _Pragma("clang attribute pop B")
#define C_BEGIN _Pragma("clang attribute push C (c)")
#define C_END _Pragma("clang attribute pop C")
```
To be clear, without this behaviour (or something equivalent) `#pragma clang
attribute` is completely broken for its motivating use case. If we can't find
a design that allows us to interleave these macros, we will have to abandon
this pragma entirely (and go back to adding a significant number of dedicated
one-off pragmas).
But it's the behaviour we care about, not this particular syntax. Since this
pragma is designed specifically to be hidden behind macros, it can be as ugly
as you want. Is there another way of spelling this that you find more
intuitive? Or should we just improve the docs as Erik has done?
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
