erichkeane added inline comments.

================
Comment at: clang/test/Sema/attr-alwaysinline.cpp:36
+        return x;
+}
+
----------------
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.


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

Reply via email to