Mordante marked an inline comment as done.
Mordante added inline comments.
================
Comment at: clang/lib/Sema/SemaStmt.cpp:578
+static std::pair<Stmt::Likelihood, const Attr *>
+getLikelihood(const Stmt *Stmt) {
+ if (const auto *AS = dyn_cast<AttributedStmt>(Stmt))
----------------
aaron.ballman wrote:
> Mordante wrote:
> > rsmith wrote:
> > > Mordante wrote:
> > > > aaron.ballman wrote:
> > > > > rsmith wrote:
> > > > > > Sema doesn't care about any of this; can you move this code to
> > > > > > CodeGen and remove the code that stores likelihood data in the AST?
> > > > > FWIW, I asked for it to be moved out of CodeGen and into Sema because
> > > > > the original implementation in CodeGen was doing a fair amount of
> > > > > work trying to interrogate the AST for this information. Now that
> > > > > we've switched the design to only be on the substatement of an
> > > > > if/else statement (rather than an arbitrary statement), it may be
> > > > > that CodeGen is a better place for this again (and if so, I'm sorry
> > > > > for the churn).
> > > > At the moment the Sema cares about it to generate diagnostics about
> > > > conflicting annotations:
> > > > ```
> > > > if(i) [[ likely]] {}
> > > > else [[likely]] {}
> > > > ```
> > > > This diagnostic only happens for an if statement, for a switch multiple
> > > > values can be considered likely.
> > > > Do you prefer to have the diagnostic also in the CodeGen?
> > > It looked to me like you'd reached agreement to remove that diagnostic.
> > > Are you intending to keep it?
> > >
> > > If so, then I'd suggest you make `getLikelihood` a member of `Stmt` so
> > > that `CodeGen` and the warning here can both easily call it.
> > @aaron.ballman I thought we wanted to keep this conflict warning, am I
> > correct?
> > I'll add an extra function to the Stmt to test for a conflict and use that
> > for the diagnostic in the Sema. This allows me to use `LH_None` for no
> > attribute or a conflict of attributes in the CodeGen. Then there's no need
> > for `LH_Conflict` and it can be removed from the enumerate.
> > @aaron.ballman I thought we wanted to keep this conflict warning, am I
> > correct?
>
> I want to keep the property that any time the user writes an explicit
> annotation in their code but we decide to ignore the annotation (for whatever
> reason), the user gets some sort of diagnostic telling them their
> expectations may not be met. Because we're ignoring the attributes in the
> case where they're identical on both branches, I'd like to keep some form of
> diagnostic.
Good then we're on the same page.
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
