[PATCH] D43564: TableGen: Introduce an abstract variable resolver interface

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

nhaehnle added a comment.

Thanks for taking a look at all the patches!



================
Comment at: include/llvm/TableGen/Record.h:1631
+
+  virtual bool keepUnsetBits() const { return false; }
+};
----------------
tra wrote:
> Please describe the purpose of this function. It's not clear what are those unset bits and why would we want (or not) to keep them.
It's to do with keeping cross-references alive to represent instruction encodings. I'm adding a comment, see also http://nhaehnle.blogspot.de/2018/02/tablegen-3-bits.html


================
Comment at: include/llvm/TableGen/Record.h:1642
+
+  Init *resolve(Init *VarName) override;
+
----------------
tra wrote:
> This needs documentation, too. Does it accept nullptr? What does it return? Can it return nullptr? If so, what does it mean? 
This is an override and we don't usually duplicate documentation for those in LLVM. The base class has documentation.


================
Comment at: lib/TableGen/Record.cpp:1837-1858
+Init *RecordResolver::resolve(Init *VarName) {
+  Init *Val = Cache.lookup(VarName);
+  if (Val)
+    return Val;
+
+  for (Init *S : Stack) {
+    if (S == VarName)
----------------
tra wrote:
> Do we ever cache objects that get deallocated? 
> If so, using pointers for cache is problematic VarName may have been deallocated and re-allocated to represent something else since it was cached. Perhaps you want to use fully qualified var name as the key instead.
Inits are never deallocated (except on program shutdown). They are owned by the static pools from which they are allocated, see the various ::get function implementations.


Repository:
  rL LLVM

https://reviews.llvm.org/D43564