[PATCH] D78938: Make LLVM build in C++20 mode

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:40
   /// Static polymorphism: delegate implementation (via isEqualTo) to the
   /// derived class.
+  bool operator==(const DGEdge &E) const {
----------------
jfb wrote:
> That comment, so informative! 😐
It would be better to make these non-member functions as well, to be consistent with the `DGNode`.
```
  friend bool operator==(const EdgeType &Elhs, const EdgeType &Erhs) {
    return Elhs.isEqualTo(Erhs);
  }
  friend bool operator!=(const EdgeType &Elhs, const EdgeType &Erhs) {
    return !(Elhs == Erhs);
  }
```


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:40
   /// Static polymorphism: delegate implementation (via isEqualTo) to the
   /// derived class.
+  bool operator==(const DGEdge &E) const {
----------------
bmahjour wrote:
> jfb wrote:
> > That comment, so informative! 😐
> It would be better to make these non-member functions as well, to be consistent with the `DGNode`.
> ```
>   friend bool operator==(const EdgeType &Elhs, const EdgeType &Erhs) {
>     return Elhs.isEqualTo(Erhs);
>   }
>   friend bool operator!=(const EdgeType &Elhs, const EdgeType &Erhs) {
>     return !(Elhs == Erhs);
>   }
> ```
> That comment, so informative! 😐

Yeah, it's not the best comment. It is trying to say that //the `isEqualTo` function gets selected, based on the type of the derived class in the CRTP, and that the selection is done at compile-time using type information, rather than at runtime using dynamic polymorphism//. Please feel free to update it to say that or offer any other suggestions you might have.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78938/new/

https://reviews.llvm.org/D78938