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

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham marked 8 inline comments as done.
simon_tatham added inline comments.


================
Comment at: llvm/docs/TableGen/LangRef.rst:430
+The identifier on the left of the ``=`` is defined to be a global or
+multiclass-local variable, whose value is given by the expression on the right
+of the ``=``. The type of the variable is automatically inferred.
----------------
hfinkel wrote:
> or class/record local
> 
> or should we just say a "scope-local variable" (including some kind of global scope)
Yes – I didn't revisit these docs after rethinking the scoping system. I've rewritten the whole section to try to be more specific about what does and doesn't work.


================
Comment at: llvm/lib/TableGen/TGParser.h:88-90
+    auto OldParent = std::move(parent);
+    parent = nullptr;
+    return std::move(OldParent);
----------------
nhaehnle wrote:
> This feels unidiomatic to me.
Hmm. I did that because I wasn't confident that doing a `std::move` out of a `unique_ptr` would leave it in a well defined null-pointer state – my reading of the C++ standard is that generally a moved-from library object is in a state where you can safely destruct or assign over it but otherwise unspecified, and `unique_ptr` isn't an exception as far as I know.

But looking again, of course the point of `extractParent` is precisely that we're //about// to destruct this object and its contained `unique_ptr`, so there's no reason I can't just `std::move` directly out of `parent` in the more obvious way, and not worry about what state it's left in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71407