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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:1693
+The attributes are used to aid the compiler to determine which branch is
+likely or unlikely to be taken. This is done by marking the first statement in
+the branch with one of the two attributes.
----------------
How about: `This is done by marking the branch substatement with one of the two attributes.`


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


================
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
----------------
How about: `In Clang, the attributes will be ignored if they're not placed on the substatement of a selection 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():
----------------
The formatting on this comment looks off (maybe different indentation, or tabs vs spaces).


================
Comment at: clang/include/clang/Basic/AttrDocs.td:1736
+
+At the moment the attribute only has effect when used in an ``if`` statement.
+
----------------
`if` or `else` statement.


================
Comment at: clang/lib/AST/JSONNodeDumper.cpp:1452
   attributeOnlyIfTrue("isConstexpr", IS->isConstexpr());
+  dumpLikelihood(JOS, "thenLikelihood", IS->getThenLikelihood());
 }
----------------
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.


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:927
     OS << " has_else";
+  dumpLikelihood(OS, Node->getThenLikelihood());
 }
----------------
Similar here.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:618
+
+  // The function returs the likelihood of Then so invert the likelihood of
+  // Else.
----------------
returs -> returns


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

https://reviews.llvm.org/D85091