dexonsmith added inline comments.
================ Comment at: clang/test/Sema/pragma-attribute-label.c:15 +// Out of order! +#pragma clang attribute pop MY_LABEL + ---------------- aaron.ballman wrote: > dexonsmith wrote: > > 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? > > As Erik says, the intention is that these are only used by macros. > > That's a nice intention, but that doesn't mean all users are going to do > that. I'm worried about the users who don't adhere to that intention and use > the pragmas in their more exposed format, given that it's a supported syntax. > > > > 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. > > I'm not worried about the ugliness, I'm worried about whether it's > understandable behavior when the pragma is not hidden behind a macro. > > > Is there another way of spelling this that you find more intuitive? Or > > should we just improve the docs as Erik has done? > > @erik.pilkington and I talked over IRC and I have a better understanding of > where my disconnect is here. It seems as though push/pop was the incorrect > initial design for the feature and what we really wish we had was a > region-based approach rather than a stack-based approach. Would it make sense > to add `#pragma clang attribute begin/end` syntax with labels instead? Then > the macros could switch to using that approach to solve the problems you're > running into. We could still have push/pop with labels, but they could warn > on label mismatches (which would also help alert users to problems like what > is in the summary for this patch) instead of treating it as a region. Sounds great; thanks Aaron. Another idea I had over lunch was `#pragma clang attribute IDENTIFIER push/pop`, making it clear that each identifier has an independent stack. 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
