mizvekov added inline comments.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:11815
+ if (IsError)
+ return Opc != BO_Cmp ? Context.getLogicalOperationType() : QualType();
+ }
----------------
Quuxplusone wrote:
> Peanut gallery says: Is `QualType()` the right "placeholder" to return here?
> IIUC, this is the situation where we've diagnosed an ill-formed expression
> and are just trying to do error-recovery: if the expression looks like `x <
> y` then we assume the programmer wants it to return `bool`, and if the
> expression looks like `x <=> y` then we assume the programmer wants it to
> return... `QualType()`? Is that the same thing we'd do for e.g. `x + y` or
> `undeclaredfunction(x)`? (If so, good, I think.)
So right now, this is the same we are doing for three-way comparison between
builtin types we do not support / recognize. Check just below in this same
function. Just look for the call to `getComparisonCategoryForBuiltinCmp`.
I agree completely with you and I was going to raise the same point, but I
would prefer we changed both places at once instead of fixing it just here, so
I think this should be a follow up task: Find something more appropriate to
return here.
================
Comment at: clang/test/Parser/cxx-template-argument.cpp:28
+ (void)(&t<int>==p); // expected-error {{use '> ='}}
+ (void)(&t<int>>=p); // expected-error {{use '> >'}} expected-error
{{ordered comparison of function pointers}}
+ (void)(&t<S<int>>>=p); // expected-error {{ordered comparison of function
pointers}}
----------------
Quuxplusone wrote:
> mizvekov wrote:
> > So here we are recovering from the parser error into this type check error.
> > Maybe there is something that could be improved as a follow up task so we
> > don't get a double error.
> Since this is a parsing test, not a semantics test, I think it should avoid
> doing anything sketchy semantic-wise. It should just be rewritten as
> something like
> ```
> struct RHS {
> friend void operator==(void(*)(), RHS) {}
> friend void operator>=(void(*)(), RHS) {}
> };
> (void)(&t<int>==RHS());
> (void)(&t<int>>=RHS());
> (void)(&t<S<int>>==RHS());
> (void)(&t<S<int>>>=RHS());
> ```
I like that idea.
================
Comment at: clang/test/SemaCXX/compare-function-pointer.cpp:12
+bool ge = a >= b; // expected-error {{ordered comparison of function
pointers}}
+bool tw = a <=> b; // expected-error {{ordered comparison of function
pointers}}
----------------
Quuxplusone wrote:
> I believe you should also test these same cases for `a OP c` where `c` is a
> different function pointer type, e.g. `int (*c)();`
Sounds good.
================
Comment at: clang/test/SemaTemplate/resolve-single-template-id.cpp:73-75
+ oneT<int> < oneT<int>; // expected-warning {{self-comparison always
evaluates to false}} \
+ // expected-warning {{relational comparison result
unused}} \
+ // expected-error {{ordered comparison of function
pointers}}
----------------
Quuxplusone wrote:
> Cast `(void)(x < y)` here to suppress one of these irrelevant warnings.
> The combination of warning "expr always evaluates to false" and erroring
> "expr is ill-formed" is also silly, but I suppose we can't do much about it.
I tried avoid changing the original test because I am not sure what the
original intention was, but I agree in principle the two errors already give
enough indication that the compiler is figuring out what is happening here
correctly.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104680/new/
https://reviews.llvm.org/D104680
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
