[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 8 inline comments as done.
Mordante added a comment.

Thanks again for your feedback. Once we agree on what to do with the Likelihood state in the output I'll finish the changes and submit a new patch.



================
Comment at: clang/include/clang/Basic/AttrDocs.td:1697
+It isn't allowed to annotate a single statement with both ``likely`` and
+``unlikely``. It's allowed to create a contradictions by marking
+the first statement in the ``true`` and ``false`` branch with the same
----------------
aaron.ballman wrote:
> This sentence and the next one seem to say opposite things. I think this should read:
> 
> `Annotating the 'true' and 'false' branch of an 'if' statement with the same likelihood attribute will result in a diagnostic and the attributes are ignored on both branches.`
I think the current wording is correct, but I like your wording better. So I'll use your wording.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:1705
+
+In Clang the attributes will be ignored if they're not placed on the first
+statement in a branch. The C++ Standard recommends to honor them on every
----------------
aaron.ballman wrote:
> How about: `In Clang, the attributes will be ignored if they're not placed on the substatement of a selection statement.`
Your wording is correct for an `if` statement, but it will be different in a `switch` statement. I'll use your wording but replace `selection statement` with `if or else statement`. I like the substatement instead of first statement.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:1726
+    foo(b);
+	// Whether or not the next statement is in the path of execution depends
+    // on the declaration of foo():
----------------
aaron.ballman wrote:
> The formatting on this comment looks off (maybe different indentation, or tabs vs spaces).
Good catch, I thought I disabled using tabs.


================
Comment at: clang/lib/AST/JSONNodeDumper.cpp:1452
   attributeOnlyIfTrue("isConstexpr", IS->isConstexpr());
+  dumpLikelihood(JOS, "thenLikelihood", IS->getThenLikelihood());
 }
----------------
aaron.ballman wrote:
> I don't think this change should be necessary because the attribute should be written out for the AST node already. It's worth verifying though.
They are in the AST. I added them to make it clear what the status is. I marked a case with a conflict, due to the same attribute in both branches. When the attribute is there but not on the substatement it's ignored. I'm not sure whether this is too important in the JSON output. I don't know who consumes it. So I don't feel to strongly about removing this from the JSON output.

What's your opinion?


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:927
     OS << " has_else";
+  dumpLikelihood(OS, Node->getThenLikelihood());
 }
----------------
aaron.ballman wrote:
> Similar here.
I'm against removing it from the text output. This is be best way to determine how the attributes are parsed and processed. Especially when an attribute is ignored the likelihood may be different form what's expected. The `Node->hasInitStorage()`'s status is also visible in the AST and written out, so there are more examples of "duplicated" information.

Do you agree to keep it here?


================
Comment at: clang/test/AST/ast-dump-if-json.cpp:1333
+// CHECK-NEXT:  "hasElse": true,
+// CHECK-NEXT:  "thenLikelihood": "conflict",
+// CHECK-NEXT:  "inner": [
----------------
Here is the likelihood status in the `If` statement.


================
Comment at: clang/test/AST/ast-dump-if-json.cpp:1429
+// CHECK-NEXT:      "id": "0x{{.*}}",
+// CHECK-NEXT:      "kind": "UnlikelyAttr",
+// CHECK-NEXT:      "range": {
----------------
The attribute in the `ThenStmt`.


================
Comment at: clang/test/AST/ast-dump-if-json.cpp:1483
+// CHECK-NEXT:      "id": "0x{{.*}}",
+// CHECK-NEXT:      "kind": "UnlikelyAttr",
+// CHECK-NEXT:      "range": {
----------------
The attribute in the `ElseStmt`.


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

https://reviews.llvm.org/D85091