<table border="1" cellspacing="0" cellpadding="8">
    <tr>
        <th>Issue</th>
        <td>
            <a href=https://github.com/llvm/llvm-project/issues/79275>79275</a>
        </td>
    </tr>

    <tr>
        <th>Summary</th>
        <td>
            Handling of spaceship operator seems to be dangerously inconsistent in AST/Expr.h
        </td>
    </tr>

    <tr>
      <th>Labels</th>
      <td>
            new issue
      </td>
    </tr>

    <tr>
      <th>Assignees</th>
      <td>
      </td>
    </tr>

    <tr>
      <th>Reporter</th>
      <td>
          NagyDonat
      </td>
    </tr>
</table>

<pre>
    The C++20 spaceship operator (`<=>`, `OP_Cmp`) is categorized as a comparison operator by `BinaryOperator::isComparisonOperator()`, however, passing it to `negateComparisonOp` or `reverseComparisonOp` would run into `llvm_unreachable("Not a comparison operator.");`.

Currently use of these negate/reverse functions is guarded by awkward checks like
```cpp
if (!BinaryOperator::isComparisonOp(CurrentOP) || (CurrentOP == BO_Cmp))
  return std::nullopt; 
```
or explicit enumeration of all comparison operators (e.g. as `case` labels).

Unfortunately, this is a very error-prone pattern, in fact it seems that within `RangedConstraintManager::assumeSym` the first half of the function (the case of `if (const SymIntExpr *SIE = dyn_cast<SymIntExpr>(Sym))`) contains an `&& op != BO_Cmp` check, but this is forgotten/omitted in the second half (the case of `if (const auto *SSE = dyn_cast<SymSymExpr>(Sym))`, which also uses `isComparisonOp()` followed by calls to both `negateComparisonOp` and `reverseComparisonOp`).

I found this issue when I wanted to use `isComparisonOperator()` and I double-checked its implementation (I almost assumed that it does the "right thing", and then I would've written code that crashes on the spaceship). This means that I don't have a reproducer where this issue actually leads to a crash, but I think that it's still important to provide a systemic solution that removes this footgun.

The main problem is that the C++ standard committee said that `<=>` is a comparison operator. I understand that this is helpful for human reader (who expects to find it in the same section of the document as all the other comparison operators), but this makes the standard-following `isComparisonOperator()` almost useless in the compiler development, where `<=>` lacks many properties that are shared by the common comparison operators (e.g. its return type isn't bool, its negation or reversal cannot be expressed as another operator).

Personally I'd prefer a radical solution that eliminates the "poisoned" word `Comparison` and uses something like `RelationalOrEquality` that's ugly and more verbose but won't lead to a constant trickle of bugs. 

</pre>
<img width="1px" height="1px" alt="" src="http://email.email.llvm.org/o/eJyEVl1v4zoO_TXKCzGBqzQffchDm06xedjbwXb2ecBYtK2tLHklqrneX7-g7LSdTudeIGgTf5CHh-eQwpRs64n2an2n1vcLzNyFuP8D2_E-eOTFKZhx_70jOCh9p_SdriANWFPq7ABhoIgcIii9U5tKrQ5qda9WX-W7PoDaVI_ffhz6ofy-AZugRqY2RPs_MoAJEOrQDxhtCv4t2mmUV--sxzg-zhfV6latbm06vD7_ekfvlL6ZU3bhTC8U5esgtfkWLAMHCeipRab3AdSmAkG_qaK8lX65eQ7ZGYjZg_VTEOde-h_ZR8K6w5Ojkl3_EfjzWpZKa0G3ulObaqmqe1XdTn8POUby7EbIiSA0wB0lggmk0g8zImiyr9kGn4S-NmM0ZIQhPD-fMRqoO6qfEzj7THP4TTV96mGYrtimNEhf_T2lSu9mYI_fpGVqe1DbA7y_DKXH93D3WFqrb-RT8gBE4hw9JDZTcJ-dCwOr1R18ADf9DBHoz8HZ2jKQz70As0JfA-jcZ4QmwULLdinykSIxkXTK4YlcUvrmJ5L_7ZsQOXtkcqOIgjtbiER4oTgCxRjilyEGTzAgM0UvT1kPDdYs0klEfQLukOFsubNekv4LfUvmEHziiNbzP9FjSzOhmFLu6WnsBRZ3BI2NiaFD18xtfu2p1CK_pQi5pzbV1KtaQsPT2B89f_1zEIPdPh2_CvVgRv-jxsRqdXh7QEynd5J06sdkuDp4RusTYIGt9EbpDYQBlL5618JNNalISj9lfiWpCbENzOSVfgi9ZSYj1AjiRHXwZirqr4vALMbRt09Pn8F_GvvfwT_AubN1B-hSEI-Udv-i1ulpaIJz4Tw5o0bnknj-FLj7vfHRm987_6OQjtCE7M2FmpQJzh15OMIZvfDCBeOvED_MqJL1CCbkk6MvhXUhlRPYfnDUk2e8KOMI6PogFBZFmUmFlsEESqULSuto2650zLdl1BxKBp6xyfhSevtCcI7SPw91MDQFqiOmjhKEuaOXqS6lw3epsyf0s_YFsld6Kzp-IUCINMRgck1RiIj0nhmsOaNzIzhCUzqBU7aLwI4F8POlIKW3CRJb54SFEBl9mdlDDC_WSLY0Jqbe1pCCy4Wf8mqkPrwULopaA7fZ_9Q2WVw9Wi-hTo56UXV5k982GiRGb8ooDX1ROUFCO9P9Ya1Ns-OzQQ9HyN5QLNEuSSYfdeSGJjvxE3S5Rw-R0FBZm-cuyAikmgtRjfUih1ebYV-8dhmKcs2EOotOyv50rlwL3FH8dFwWQ72zdY_Ps3guZX-ZvCOr8u_VOwkyJ3KU0gWl5LWOIhh6IRcGQTf5V4TxkUGHsq969KN0ZaDIluauYCRIHcbJx3PoPvi_XgTinnnx8DgQ2DQp9RSCK9Oc07RWC4kRJsejgxq9Dwwnkg5ESmk-lPiJzvBa_s-z4BvFFHwR-FHprYEhUkNRTIHG1ug-qJSc7a2soFfTDkFKIaO0hnOIZQ690X4ZE2XmpdBTcXfZ8GX5kCuloHuMX_-b0Vkep1WDk5Ny68YSoA-RZM-dQqKigPPsYfHlbEuZ0cVu0dbPrgzwU27Tct7XC7NfmZvVDS5of7WtNtXNer3aLbr9el3RTtPuVK-3681226yvTs11tcLdjprddruwe13p6-pKX19d6fVqt9wRmatqpU81Vdhsd-q6oh6tW8qRahliuyjTY7-90dv1Ytrn5ViqtafzNFpkxK3vF3Ev73wRoOq6cjZxeovClh3t_4HeOKEtNJ8dWOe1HqT5RrZ5DDm5EawXRmxiMZj1cPv0XekHWVHLbpGj23fMQ5JFrx-Ufmgtd_m0rEOv9IMgmP_JkeI_VLPSDwV3Uvqh1PX_AAAA__8jstZE">