[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Mar 22 07:04:33 PDT 2019


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





More information about the cfe-commits mailing list