aaron.ballman added a reviewer: rsmith.
aaron.ballman added a comment.
Adding Richard for design strategy discussion.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8164
+def err_must_appear_after_branch : Error<
+ "%0 can only appear after a selection or iteration statement">;
+def warn_attribute_already_present : Warning<
----------------
riccibruno wrote:
> I don't think that this is right. I don't see this restriction in the
> specification. A warning should definitely be emitted when the attribute is
> ignored, but I believe that an error is inappropriate.
> I don't think that this is right. I don't see this restriction in the
> specification. A warning should definitely be emitted when the attribute is
> ignored, but I believe that an error is inappropriate.
Agreed.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8165
+ "%0 can only appear after a selection or iteration statement">;
+def warn_attribute_already_present : Warning<
+ "was already marked %0">;
----------------
This diagnostic isn't needed -- `warn_duplicate_attribute_exact` will cover it.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8167
+ "was already marked %0">;
+def err_mutuably_exclusive_likelihood : Error<
+ "%0 and %1 are mutually exclusive">;
----------------
This diagnostic is not needed -- `err_attributes_are_not_compatible` will cover
it.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8170
+def err_contradictory_attribute : Warning<
+ "%0 contradicing with previous attribute">;
+
----------------
riccibruno wrote:
> This should be `warn_contradictory_attribute`, and I am not seeing tests for
> this.
This note also isn't needed. You should use `note_conflicting_attribute`
instead.
================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:64-75
+ if (CurScope) {
+ Scope* ControlScope = CurScope->getParent();
+ if (!ControlScope ||
+ !(ControlScope->getFlags() & Scope::ControlScope ||
+ ControlScope->getFlags() & Scope::BreakScope) ||
+ CurScope->getFlags() & Scope::CompoundStmtScope ||
+ ControlScope->getFlags() & Scope::SEHExceptScope ||
----------------
This is incorrect -- nothing in the standard requires the attribute to appear
after a branch statement. I'm not even 100% convinced that this should map
directly to `__builtin_expect` in all cases (though it certainly would for
conditional branches), because that doesn't help with things like catch
handlers or labels.
================
Comment at: clang/test/SemaCXX/cxx2a-likelihood-attr.cpp:52
+}
\ No newline at end of file
----------------
Be sure to add the newline, please.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59467/new/
https://reviews.llvm.org/D59467
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
