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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel abandoned this revision.
ymandel marked an inline comment as done.
ymandel added inline comments.


================
Comment at: clang/include/clang/Tooling/Refactoring/NodeId.h:29
+public:
+  explicit NodeId(std::string Id) : Id(std::move(Id)) {}
+
----------------
ilya-biryukov wrote:
> 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.
If there are errors relating to the id (say, referencing it in a stencil when it wasn't bound in the match), the errors are hard to understand when the id is auto-generated. In general, I'd say that best practice is to explicitly specify the id so that dynamic failures print a meaningful id.  The default constructor is a compromise and perhaps shouldn't be exported.

that said, if we could find another way to give good error messages (for example, if we could somehow grab the line number of the declaration), then the default construction would be the preferred approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59329