[PATCH] D156244: [clang] Do not crash on use of a variadic overloaded operator

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:14001
+
+        if (FnDecl->isInvalidDecl())
+          return ExprError();
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156244