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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D59467#1443173 <https://reviews.llvm.org/D59467#1443173>, @Tyker wrote:

> @lebedev.ri where are tests for AST serialization are located ? i didn't find them.


They live in clang\tests\PCH.

In D59467#1440608 <https://reviews.llvm.org/D59467#1440608>, @Tyker wrote:

> i implemented the semantic the changes for if for, while and do while statement and the AST change to if. can you review it and tell me if it is fine so i implement the rest. i didn't update the test so they will fail.


I think this may be moving closer to the correct direction now, but I'm still curious to hear if @rsmith has similar opinions.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8164
+def warn_no_associated_branch : Warning<
+  "attribute %0 not assiciated to any branch is ignored">, InGroup<IgnoredAttributes>;
+def warn_conflicting_attribute : Warning<
----------------
Typo: associated

Should probably read "attribute %0 is not associated with a branch and is ignored"

Also, I'd rename the diagnostic to be `warn_no_likelihood_attr_associated_branch`


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8165-8166
+  "attribute %0 not assiciated to any branch is ignored">, InGroup<IgnoredAttributes>;
+def warn_conflicting_attribute : Warning<
+  "%0 contradicing with previous %1 attribute">, InGroup<IgnoredAttributes>;
+
----------------
I'd rename the diagnostic to `warn_conflicting_likelihood_attrs` and reword the diagnostic to "attribute %0 conflicts with previous attribute %1"


================
Comment at: clang/include/clang/Sema/Scope.h:163
 
+  /// BranchAttr - Likelihood attribute associated with this Branch or nullptr
+  Attr *BranchAttr;
----------------
Missing full stop at the end of the comment. You should check all the comments in the patch to be sure they are full sentences (start with a capital letter, end with punctuation, are grammatically correct, etc).


================
Comment at: clang/include/clang/Sema/Scope.h:164
+  /// BranchAttr - Likelihood attribute associated with this Branch or nullptr
+  Attr *BranchAttr;
+
----------------
Rather than store an `Attr` here, would it make sense to instead store a `LikelihoodAttr` directly? That may clean up some casts in the code.


================
Comment at: clang/include/clang/Sema/Sema.h:3845
+  /// emit diagnostics and determinate the hint for and If statement
+  BranchHint HandleIfStmtHint(Stmt *thenStmt, Stmt *elseStmt, Attr *ThenAttr,
+                              Attr *ElseAttr);
----------------
You should check all of the identifiers introduced by the patch to ensure they meet our coding style requirements (https://llvm.org/docs/CodingStandards.html). `thenStmt` -> `ThenStmt`, etc.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:526
+                                  Attr *ThenAttr, Attr *ElseAttr) {
+  // diagnose branch with attribute and null statement as empty body
+  if (thenStmt && isa<AttributedStmt>(thenStmt) &&
----------------
The condition here is incorrect -- it's not checking what kind of attributed statement is present to see if it's a likelihood statement.

However, what is the value in diagnosing this construct? It seems not-entirely-unreasonable for a user to write, for instance:
```
if (foo) {
  [[likely]];
  ...
} else [[unlikely]];
```


================
Comment at: clang/lib/Sema/SemaStmt.cpp:548
+    } else {
+      if (ThenAttr->getSpelling()[0] == 'l')
+        Hint = Taken;
----------------
This is fragile -- you should do something more like:
```
if (const auto *LA = dyn_cast<LikelihoodAttr>(ThenAttr)) {
  Hint = LA->isLikely() ? ... : ...;
}
```
You'll want to add an accessor onto the LikelihoodAttr object in Attr.td to implement the `isLikely()` (and `isUnlikely()`) functions based on the attribute spelling. There are examples of this in Attr.td.


================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:66-73
+    if (!ControlScope ||
+        !(ControlScope->getFlags() & Scope::ControlScope ||
+          ControlScope->getFlags() & Scope::BreakScope) ||
+        ControlScope->getFlags() & Scope::SEHExceptScope ||
+        ControlScope->getFlags() & Scope::SEHTryScope ||
+        ControlScope->getFlags() & Scope::TryScope ||
+        ControlScope->getFlags() & Scope::FnTryCatchScope) {
----------------
This doesn't seem to account for `switch` statements, like this, does it?
```
switch (foo) {
[[likely]] case 1: break;
[[unlikely]] case 2: break;
[[likely]] case 3: break;
[[unlikely]] default: break;
}
```
Note, it's perfectly reasonable for there to be repeated attributes here because some cases may be more likely than others.


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

https://reviews.llvm.org/D59467