[PATCH] D59329: [LibTooling] Add NodeId, a strong type for AST-matcher node identifiers.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a subscriber: klimek.
ilya-biryukov added inline comments.


================
Comment at: clang/include/clang/Tooling/Refactoring/NodeId.h:29
+public:
+  explicit NodeId(std::string Id) : Id(std::move(Id)) {}
+
----------------
What are the use-cases for passing a custom id to this class?
If the purpose is to hide the IDs from the user, exposing this constructor seems to be against this goal.


================
Comment at: clang/lib/Tooling/Refactoring/NodeId.cpp:12
+
+namespace clang {
+namespace tooling {
----------------
NIT: change to `using namespace` for consistency


================
Comment at: clang/lib/Tooling/Refactoring/NodeId.cpp:24
+
+NodeId::NodeId() : NodeId(nextId()) {}
+
----------------
Suggestion from @klimek :

could we get the unique string by printing a pointer of `this` instead?
(And disallow copies and moves of this class for this to be correct)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59329