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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
