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'}}
----------------
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?
================
Comment at: clang/test/Sema/pragma-attribute-label.c:15
+// Out of order!
+#pragma clang attribute pop MY_LABEL
+
----------------
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.
Repository:
rC Clang
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
