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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


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


================
Comment at: clang/test/SemaCXX/overloaded-operator-decl.cpp:64
+class E {};
+void operator+(E, ...) {} // expected-error{{overloaded 'operator+' cannot be variadic}}
+void d() { E() + E(); }
----------------
I think it might make sense to extend the test coverage for the other operators you can overload, just to demonstrate we diagnose them all consistently. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156244