[PATCH] D71407: [TableGen] Introduce a `defvar` statement.

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

nhaehnle added a comment.

In D71407#1781506 <https://reviews.llvm.org/D71407#1781506>, @simon_tatham wrote:

> - should a defvar in a foreach be local to it? (Probably)
> - should I allow defvar inside a class or def as well as in a multiclass, defining variables that vanish at the closing brace but can be referred to by the definitions of proper class member values? (Possibly)
> - is defvar the sensible name? (`def` and `let` were already taken; I considered C++ `using`, but thought `defvar` fits reasonably well with the existing `defset`)


This is a good feature, and I'd say yes to all three for consistency. Thank you for working on it! The overall design of the code looks reasonable to me, though I do have some nitpicks.



================
Comment at: llvm/lib/TableGen/TGParser.h:79-80
+  // A scope to hold local variable definitions from defvar.
+  std::map<std::string, Init *, std::less<>> Vars;
+  std::unique_ptr<TGLocalVarScope> parent;
+
----------------
Why the inconsistent capitalization? I'm fine with using lower case since there seems to be rough consensus on going there eventually.


================
Comment at: llvm/lib/TableGen/TGParser.h:83
+public:
+  TGLocalVarScope() : parent(nullptr) {}
+  TGLocalVarScope(std::unique_ptr<TGLocalVarScope> parent)
----------------
The initializer is unnecessary.


================
Comment at: llvm/lib/TableGen/TGParser.h:88-90
+    auto OldParent = std::move(parent);
+    parent = nullptr;
+    return std::move(OldParent);
----------------
This feels unidiomatic to me.


================
Comment at: llvm/lib/TableGen/TGParser.h:95-100
+    if (It != Vars.end())
+      return It->second;
+    else if (parent)
+      return parent->getVar(Name);
+    else
+      return nullptr;
----------------
Just make this a sequence of if()s (no else).


================
Comment at: llvm/lib/TableGen/TGParser.h:145
+  /// variables.
+  std::unique_ptr<TGLocalVarScope> CurLocalScope = nullptr;
+
----------------
The initializer is unnecessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71407