[PATCH] D104680: [clang] Eliminate relational function pointer comparisons in all C++ modes

[Matheus Izvekov via Phabricator via cfe-commits]

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