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

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

Additionally I think that you need to:

- add tests for templates
- handle the ternary operator

Also, as per the coding guidelines, you need to fix:

- spelling of variables, eg `hint` -> `Hint`.
- run `clang-format` on your patch.



================
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<
----------------
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.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8170
+def err_contradictory_attribute : Warning<
+  "%0 contradicing with previous attribute">;
+
----------------
This should be `warn_contradictory_attribute`, and I am not seeing tests for this.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:4022
+
+  static BranchHint getBranchHint(const Stmt* S, bool Invert = false);
+
----------------
Doc ?


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1309
+    }
+
     // Pop the 'else' scope if needed.
----------------
I don't think that this should be done here. The natural place to me seems to be in `ActOnIfStmt`. Additionally I think that you should wrap this in a static helper function.


================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:232
   for (const auto *I : Attrs) {
+
+    if (isa<LikelihoodAttr>(I)) {
----------------
A comment indicating what this is doing ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59467/new/

https://reviews.llvm.org/D59467