[PATCH] D83088: Introduce CfgTraits abstraction

[Nicolai Hähnle via Phabricator via cfe-commits]

nhaehnle added inline comments.


================
Comment at: llvm/include/llvm/Support/CfgTraits.h:51
+
+  operator bool() const { return ptr != nullptr; }
+
----------------
dblaikie wrote:
> `operator bool` should be `explicit`
Done.


================
Comment at: llvm/include/llvm/Support/CfgTraits.h:53-54
+
+  bool operator==(CfgOpaqueType other) const { return ptr == other.ptr; }
+  bool operator!=(CfgOpaqueType other) const { return ptr != other.ptr; }
+};
----------------
dblaikie wrote:
> Preferably make any operator overload that can be a non-member, a non-member - this ensures equal conversion handling on both the left and right hand side of symmetric operators like these. (they can be friends if needed, but doesn't look like it in this case - non-friend, non-members that call get() should be fine here)
Done.


================
Comment at: llvm/include/llvm/Support/CfgTraits.h:90
+/// operations such as traversal of the CFG.
+class CfgTraitsBase {
+protected:
----------------
dblaikie wrote:
> Not sure if this benefits from being inherited from, versus being freely accessible?
The idea here is to enforce via the type system that people use CfgTraits::{wrap,unwrap}Ref instead of having makeOpaque calls freely in random code.


================
Comment at: llvm/include/llvm/Support/CfgTraits.h:271-273
+template <typename CfgRelatedTypeT> struct CfgTraitsFor {
+  // using CfgTraits = ...;
+};
----------------
dblaikie wrote:
> This probably shouldn't be defined if it's only needed for specialization,  instead it can be declared:
> ```
> template<typename CfgRelatedTypeT> struct CfgTraitsFor;
> ```
Interesting. So GraphTraits should arguably be changed similarly.


================
Comment at: llvm/include/llvm/Support/CfgTraits.h:287
+public:
+  virtual ~CfgInterface() {}
+
----------------
dblaikie wrote:
> prefer `= default` where possible
Done.


================
Comment at: llvm/include/llvm/Support/CfgTraits.h:337
+    return Printable(
+        [this, block](raw_ostream &out) { printBlockName(out, block); });
+  }
----------------
dblaikie wrote:
> generally capture everything by ref `[&]` if the lambda is only used locally/within the same expression or block
The lambda is returned via `Printable`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83088