erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.
Please add the test cases requested, plus a Release Note. Otherwise LGTM.
================
Comment at: clang/test/Sema/attr-alwaysinline.cpp:36
+ return x;
+}
+
----------------
craig.topper wrote:
> 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.
>>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.
Isn't it saying that the statement diagnostic is losing to the function
attribute?
As far as that example, I wouldn't expect the diagnostic to show there, as you
mentioned.
That said, I thought this was going to be a bit more trivial than it is, I
thought this was going to be a mild drive-by. There needs to be some level of
extracting the getCallExprs checks in both cases into Sema, and have that
called upon instantiation.
I think we're OK now with this, but PLEASE make sure to put a number of tests
in for the template cases with a "FIXME".
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
