balazske marked an inline comment as done.
balazske added inline comments.
================
Comment at: clang/unittests/Tooling/SourceCodeTest.cpp:630
+ f(t);
+ // CalleeType in getCallReturntype is Overload and dependent
+}
----------------
hokein wrote:
> balazske wrote:
> > hokein wrote:
> > > CalleeType is not a specific term in `getCallReturnType`, just
> > > `CalleeType is overload and dependent`, I think we can also verify this
> > > in the Visitor.OnCall callback.
> > >
> > > IIUC, the patch fixes three different crashes
> > > - calling getCallReturntype on the `f(t)` expr
> > > - calling getCallReturntype on the `f_overload()` expr
> > > - calling getCallReturntype on the `a.f_overload(p);` expr
> > >
> > > I think it would be much clear to express them in the `OnCall` callback
> > > (rather than calling `getCallReturnType` on every Expr). One option is to
> > > use the annotation, and identify the expression by location like below.
> > > An other benefit is that we can unify the test code (instead of
> > > duplicating them) without hurting the code readability.
> > >
> > > ```
> > > template<class T, class F>
> > > void templ(const T& t, F f) {
> > > $crash1[[f]](t);
> > > // CalleeType in getCallReturntype is Overload and dependent
> > > }
> > >
> > > struct A {
> > > void f_overload(int);
> > > void f_overload(double);
> > > };
> > >
> > > void f() {
> > > int i = 0;
> > > templ(i, [](const auto &p) {
> > > $crash2[[f_overload]](p); // UnresolvedLookupExpr
> > > // CalleeType in getCallReturntype is Overload and not dependent
> > > });
> > >
> > > templ(i, [](const auto &p) {
> > > A a;
> > > a.$crash3[[f_overload]](p); // UnresolvedMemberExpr
> > > // CalleeType in getCallReturntype is BoundMember and has overloads
> > > });
> > >
> > > }
> > > ```
> > >
> > The current test finds every `CallExpr` and calls the `getCallReturnType`
> > on it. The test should verify that this function works, so at least calling
> > it is needed. An additional check could be to verify that the "Callee" is
> > really of the specific kind (`UnresolvedLookupExpr` and the others), this
> > can be added. I do not get it how the annotations can be used here, it is
> > possible only to get the position of the code in the string but how to use
> > this value?
> yes, I'd like to avoid the current test pattern where we invoke
> `getCallReturnType` on every `CallExpr`, and don't verify the call-expr kind.
>
> you can find the corresponding expr by matching the annotation locations,
> mostly can be done in the `OnCall` callback, like
>
> ```
> Visitor.OnCall = [](..Expr) {
> // check if location of Expr is one of the annotation locations
> // - no: return
> // - yes: find out the which one -- let's say, crash2, then we know the
> Expr should be UnresolvedLookupExpr, and the CalleeType is Overload and not
> dependent, and verify them.
> };
> ```
I am not familiar with this kind of tests but updated the code to check
`CallExpr` at the affected places only.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95244/new/
https://reviews.llvm.org/D95244
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
