aaron.ballman added inline comments.

================
Comment at: clang/lib/Sema/SemaOverload.cpp:14001
+
+        if (FnDecl->isInvalidDecl())
+          return ExprError();
----------------
shafik wrote:
> aaron.ballman wrote:
> > Fznamznon wrote:
> > > Fznamznon wrote:
> > > > shafik wrote:
> > > > > Fznamznon wrote:
> > > > > > shafik wrote:
> > > > > > > shafik wrote:
> > > > > > > > It feels a bit weird that we are succeeding in finding the best 
> > > > > > > > viable function and what we find is invalid. 
> > > > > > > It looks like we have a couple of test that generate the `cannot 
> > > > > > > be variadic` diagnostic for overloads e.g. 
> > > > > > > `clang/test/SemaCXX/overloaded-operator-decl.cpp` it might be 
> > > > > > > worth looking into why they don't crash and this case does. 
> > > > > > Yes, but it seems to be done this way in other places as well:
> > > > > > https://github.com/llvm/llvm-project/blob/0478ef2d366c6f88678e37d54190dcdaa0ec69da/clang/lib/Sema/SemaOverload.cpp#L15145
> > > > > >  .
> > > > > I see that but that diagnostic looks like it is generated by the call 
> > > > > to `CheckMemberOperatorAccess(...)` which is not really the same 
> > > > > situation we have here.
> > > > We crash when ask functiondecl its second parameter and it doesn't have 
> > > > it when the call expression is formed.
> > > > The cases from tests do not crash either because the operators are not 
> > > > used or there is at least two parameters defined in declaration of 
> > > > operator.
> > > > I see that but that diagnostic looks like it is generated by the call 
> > > > to CheckMemberOperatorAccess(...) which is not really the same 
> > > > situation we have here.
> > > 
> > > Is it? For me it seems there is a similar exit  inside of 
> > > `BuildCallToObjectOfClassType` function whose description says:
> > > ```
> > > /// BuildCallToObjectOfClassType - Build a call to an object of class
> > > /// type (C++ [over.call.object]), which can end up invoking an
> > > /// overloaded function call operator (@c operator()) or performing a
> > > /// user-defined conversion on the object argument.
> > > ```
> > > 
> > > `CheckMemberOperatorAccess` is just a call above.
> > > It feels a bit weird that we are succeeding in finding the best viable 
> > > function and what we find is invalid.
> > 
> > I don't think it's all that weird. Consider:
> > ```
> > void overloaded(int);
> > void overloaded(float) noexcept(error());
> > 
> > int main() {
> >   overloaded(1.0f);
> > }
> > ```
> > the best viable function is definitely the one taking a `float`, but the 
> > declaration itself is still invalid.
> > > It feels a bit weird that we are succeeding in finding the best viable 
> > > function and what we find is invalid.
> > 
> > I don't think it's all that weird. 
> 
> I think the fact that we have cases for which this diagnostic fires but does 
> not crash deserved some digging into to see why those cases avoid this 
> situation. If it ends up they are doing something similar then we can feel 
> more confident that this is the right approach. 
I thought we already knew what was going on there?

> The cases from tests do not crash either because the operators are not used 
> or there is at least two parameters defined in declaration of operator.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156244/new/

https://reviews.llvm.org/D156244

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to