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

[Mark de Wever via Phabricator via cfe-commits]

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