[PATCH] D78938: Make LLVM build in C++20 mode
Bardia Mahjour via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Dec 18 14:53:00 PST 2020
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
More information about the llvm-commits
mailing list