aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:1288
+def Likely : StmtAttr {
+ let Spellings = [CXX11<"", "likely", 201803>];
+ let Documentation = [LikelihoodDocs];
----------------
Mordante wrote:
> aaron.ballman wrote:
> > Mordante wrote:
> > > Mordante wrote:
> > > > aaron.ballman wrote:
> > > > > Mordante wrote:
> > > > > > Mordante wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > Mordante wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > Hmm, I'm on the fence about specifying `201803` for these
> > > > > > > > > > attributes. Given that this is only the start of supporting
> > > > > > > > > > the attribute, do we want to claim it already matches the
> > > > > > > > > > standard's behavior? Or do we just want to return `1` to
> > > > > > > > > > signify that we understand this attribute but we don't yet
> > > > > > > > > > fully support it in common cases (such as on labels in
> > > > > > > > > > switch statements, etc)?
> > > > > > > > > >
> > > > > > > > > > As another question, should we consider adding a C2x
> > > > > > > > > > spelling `[[clang::likely]]` and `[[clang::unlikely]]` to
> > > > > > > > > > add this functionality to C?
> > > > > > > > > I was also somewhat on the fence, for now I'll change it to
> > > > > > > > > specify `1`.
> > > > > > > > >
> > > > > > > > > I'll have a look at the C2x changes. Just curious do you know
> > > > > > > > > whether there's a proposal to add this to C2x?
> > > > > > > > > I'll have a look at the C2x changes. Just curious do you know
> > > > > > > > > whether there's a proposal to add this to C2x?
> > > > > > > >
> > > > > > > > There isn't one currently because there is no implementation
> > > > > > > > experience with the attributes in C. This is a way to get that
> > > > > > > > implementation experience so it's easier to propose the feature
> > > > > > > > to WG14.
> > > > > > > > I was also somewhat on the fence, for now I'll change it to
> > > > > > > > specify `1`.
> > > > > > >
> > > > > > > There seems to be an issue using the `1` so I used `2` instead.
> > > > > > > (Didn't want to look closely at the issue.)
> > > > > > >
> > > > > > > > I'll have a look at the C2x changes. Just curious do you know
> > > > > > > > whether there's a proposal to add this to C2x?
> > > > > > >
> > > > > > > There isn't one currently because there is no implementation
> > > > > > > experience with the attributes in C. This is a way to get that
> > > > > > > implementation experience so it's easier to propose the feature
> > > > > > > to WG14.
> > > > > >
> > > > > > Nice to know. It seems the C2x wasn't at straightforward as I
> > > > > > hoped, so I didn't implement it yet. I intend to look at it later.
> > > > > > I first want the initial part done to see whether this is the right
> > > > > > approach.
> > > > > What issues are you running into? 1 is the default value when you
> > > > > don't specify a value specifically.
> > > > It give the error "clang/include/clang/Basic/Attr.td:265:7: error:
> > > > Standard attributes must have valid version information."
> > > > > > I'll have a look at the C2x changes. Just curious do you know
> > > > > > whether there's a proposal to add this to C2x?
> > > > >
> > > > > There isn't one currently because there is no implementation
> > > > > experience with the attributes in C. This is a way to get that
> > > > > implementation experience so it's easier to propose the feature to
> > > > > WG14.
> > > >
> > > > Nice to know. It seems the C2x wasn't at straightforward as I hoped, so
> > > > I didn't implement it yet. I intend to look at it later. I first want
> > > > the initial part done to see whether this is the right approach.
> > >
> > > I had another look and I got it working in C.
> > If you leave the version number off entirely, it defaults to 1.
> Yes and that gives the same error. So the default value doesn't "compile". I
> assume that's intentional to avoid setting a date of 1 for standard
> attributes. So we could keep it at 2 or switch back to `201803`. Which value
> do you prefer?
Ah, yeah, you're right (sorry for the noise). I think `2` is fine for now
unless you find that the new patch actually hits enough of the important cases
from the standard to justify `201803`. We can figure that out with the updated
patch series.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:1726
+ }
+ }];
+}
----------------
Mordante wrote:
> aaron.ballman wrote:
> > Mordante wrote:
> > > aaron.ballman wrote:
> > > > Something else we should document is what our behavior is when the
> > > > attribute is not immediately inside of an `if` or `else` statement.
> > > > e.g.,
> > > > ```
> > > > void func() { // Does not behave as though specified with [[gnu::hot]]
> > > > [[likely]];
> > > > }
> > > >
> > > > void func() {
> > > > if (x) {
> > > > {
> > > > [[likely]]; // Does this make the if branch likely?
> > > > SomeRAIIObj Obj;
> > > > }
> > > > } else {
> > > > }
> > > > }
> > > >
> > > > void func() {
> > > > if (x) {
> > > > int y = ({[[likely]]; /* Does this make the if branch likely? */
> > > > 1;});
> > > > } else {
> > > > }
> > > > }
> > > > ```
> > > > Something else we should document is what our behavior is when the
> > > > attribute is not immediately inside of an `if` or `else` statement.
> > > > e.g.,
> > > > ```
> > > > void func() { // Does not behave as though specified with [[gnu::hot]]
> > > > [[likely]];
> > > > }
> > >
> > > No a few days ago I wondered whether it makes sense, but the [[gnu::hot]]
> > > should be at the declaration of the function and not in the body.
> > > So I think this doesn't make sense and the `[[likely]]` here will be
> > > ignored.
> > >
> > > > void func() {
> > > > if (x) {
> > > > {
> > > > [[likely]]; // Does this make the if branch likely?
> > > > SomeRAIIObj Obj;
> > > > }
> > > > } else {
> > > > }
> > > > }
> > >
> > > Yes this should work, the attribute will recursively visit compound
> > > statements.
> > >
> > > > void func() {
> > > > if (x) {
> > > > int y = ({[[likely]]; /* Does this make the if branch likely? */
> > > > 1;});
> > > > } else {
> > > > }
> > > > }
> > > > ```
> > >
> > > Not in this patch. I'm working on more improvements to make switch
> > > statements working. I tested it with my current WIP and there it does
> > > work.
> > > So in the future this will work.
> > > So I think this doesn't make sense and the [[likely]] here will be
> > > ignored.
> >
> > Great, that matches the behavior I was hoping for.
> >
> > > Yes this should work, the attribute will recursively visit compound
> > > statements.
> >
> > Great!
> >
> > > Not in this patch. I'm working on more improvements to make switch
> > > statements working.
> >
> > I'm not certain what that example had to do with switch statement, but just
> > as an FYI, I'd like this to work initially (assuming it's not overly hard)
> > only because GNU statement expressions show up rather frequently in macros
> > as an alternative form of the `do ... while (0);` pattern, which can have
> > likely/unlikely path sensitivity.
> It doesn't have anything to do with the switch by itself. However while
> working on the switch I needed a better way to follow the path of executed
> statements to handle `break` and `return` in a switch. As a side-effect it
> also handle the last example. So it will be possible to handle this case in a
> future patch. (However I think we should first discuss the approach to take
> in D86559.)
Ah, thank you for the explanation.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85091/new/
https://reviews.llvm.org/D85091
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
