craig.topper added inline comments.
================
Comment at: clang/test/Sema/attr-alwaysinline.cpp:36
+ return x;
+}
+
----------------
erichkeane wrote:
> craig.topper wrote:
> > erichkeane wrote:
> > > craig.topper wrote:
> > > > erichkeane wrote:
> > > > > erichkeane wrote:
> > > > > > craig.topper wrote:
> > > > > > > erichkeane wrote:
> > > > > > > > craig.topper wrote:
> > > > > > > > > erichkeane wrote:
> > > > > > > > > > Can you add a test that shows that we warn on
> > > > > > > > > > instantiation? This shouldn't be a dependent declrefexpr
> > > > > > > > > > when instantiated.
> > > > > > > > > >
> > > > > > > > > > Additionally, this would make sure that we're properly
> > > > > > > > > > propoagating `always_inline`.
> > > > > > > > > Should this warn
> > > > > > > > >
> > > > > > > > > ```
> > > > > > > > > template<int D> [[gnu::noinline]]
> > > > > > > > >
> > > > > > > > > int bar(int x) {
> > > > > > > > >
> > > > > > > > > if constexpr (D > 1)
> > > > > > > > >
> > > > > > > > > [[clang::always_inline]] return bar<D-1>(x + 1);
> > > > > > > > >
> > > > > > > > > else
> > > > > > > > >
> > > > > > > > > return x;
> > > > > > > > >
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > int baz(int x) {
> > > > > > > > >
> > > > > > > > > return bar<5>(x);
> > > > > > > > >
> > > > > > > > > }
> > > > > > > > > ```
> > > > > > > > Yes, I would expect that to warn.
> > > > > > > It looks like handleAlwaysInlineAttr only gets called once so it
> > > > > > > doesn't get called after instantiation.
> > > > > > Hmm... thats unfortunate. That means we're perhaps not
> > > > > > instantiating it correctly. I'll take some time to poke around as
> > > > > > I get a chacne.
> > > > > Ok, it looks like SemaStmt.cpp's `BuildAttributedStmt` needs to
> > > > > handle the attributes. We should some day do that automatically for
> > > > > a majority o f attributes, but for now, just adding the call there is
> > > > > sufficient.
> > > > What I do need to add?
> > > You should be able to follow what is already there, but I suspect you
> > > just need to call some sort of 'handle' function for those to do this.
> > > There is one there already youc an crib off of.
> > Can I do that as a separate patch? Fixing the crash seems like a net
> > improvement.
> I'd prefer they be done together: They are a situation where you cannot
> properly test either patch without the other one being fixed, so I believe
> they are intrinsically linked.
I thought not crashing on code that doesn't need to warn would still be an
improvement.
The warning that's being lost doesn't seem all that serious. It's just telling
that an alwaysinline statement attribute has priority over a noinline function
attribute.
We don't warn for this
```
void foo();
void bar() {
[[clang::always_inline]] foo();
}
```
If foo's definition is in another translation unit, the always_inline can't
happen without LTO. So foo is effectively noinline without LTO.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146089/new/
https://reviews.llvm.org/D146089
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
