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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I believe this has the incorrect modeling -- in the following code example, the attribute appertains to the substatement of the if statement, not the if statement itself:

  if (foo) [[likely]] {
    blah;
  }

This falls out from:  http://eel.is/c++draft/stmt.stmt#1. You model this by applying the attribute to the `if` statement, which will have the wrong semantics when you hook up the hints to the backend.



================
Comment at: clang/test/SemaCXX/cxx2a-likely-attr.cpp:27
+  switch (n) {
+    [[likely]] return; // expected-error {{likely annotation can't appear here}}
+
----------------
Why? The attribute appertains to statements, and `return` is a statement. This should be accepted, I believe.


================
Comment at: clang/test/SemaCXX/cxx2a-likely-attr.cpp:29
+
+  case 0:
+    if (1) [[likely, likely]] // expected-error {{attribute 'likely' cannot appear multiple times in an attribute specifier}}
----------------
Missing a test case that the attribute should apply to labels like this (should also have a test for non-switch labels as well).


================
Comment at: clang/test/SemaCXX/cxx2a-likely-attr.cpp:30
+  case 0:
+    if (1) [[likely, likely]] // expected-error {{attribute 'likely' cannot appear multiple times in an attribute specifier}}
+      return ;
----------------
Also missing the test case covering http://eel.is/c++draft/dcl.attr.likelihood#1.sentence-3


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

https://reviews.llvm.org/D59467