[PATCH] D72294: [mlir] Refactor ModuleState into AsmState and expose it to users.

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/IR/AsmState.h:26
+/// The IR should not be mutated in-between invocations using this state, and
+/// the IR being printed must not be at a higher level than the IR originally
+/// used to initialize this state. This means that if a child operation is
----------------
jpienaar wrote:
> What does higher level mean here? It is a bit overloaded term and I think you have a very explicit meaning in mind.
Switched to 'parent'.


================
Comment at: mlir/include/mlir/IR/AsmState.h:41
+  /// A pointer to allocated storage for the impl state.
+  std::unique_ptr<detail::AsmStateImpl> impl;
+};
----------------
jpienaar wrote:
> So the lifetime of the AsmStateImpl is linked to this object, and unique_ptr is used to enable only forward declaring it above?
Yes, this is the pImpl idiom.


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:2201
+  // name and not the shadowed name.
+  state.getImpl().getSSANameState().printValueID(*this, /*printResultNo=*/true,
+                                                 os);
----------------
jpienaar wrote:
> How is guaranteed that getImpl doesn't return null here? (e.g., verify that AsmState has been initialized)
Ooops, meant to delete the default constructor. Thanks for the catch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72294