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
