Mordante marked 7 inline comments as done.
Mordante added inline comments.
Herald added a subscriber: danielkiss.
================
Comment at: clang/include/clang/Basic/Attr.td:1288
+def Likely : StmtAttr {
+ let Spellings = [CXX11<"", "likely", 201803>];
+ let Documentation = [LikelihoodDocs];
----------------
aaron.ballman wrote:
> 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.
For now let's keep it at `2`, this patch won't implement the `switch`. Once the
`switch` works the iteration statements still need to be done, but I think they
aren't as important as the selection statements.
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
