[PATCH] D104680: [clang] Eliminate relational function pointer comparisons in all C++ modes
Arthur O'Dwyer via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 22 08:49:42 PDT 2021
Quuxplusone added a comment.
> There isn't any wording on it or anything, yet, but we must jump the gun here and just do away with it pronto.
I think "must" is the wrong word here. "Might as well"? Anyway, I agree with this general idea, FWLIW.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:11815
+ if (IsError)
+ return Opc != BO_Cmp ? Context.getLogicalOperationType() : QualType();
+ }
----------------
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.)
================
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}}
----------------
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());
```
================
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}}
----------------
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)();`
================
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}}
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104680/new/
https://reviews.llvm.org/D104680
More information about the cfe-commits
mailing list