[PATCH] D70190: [clang][modules] Add support for merging lifetime-extended temporaries

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Functionally, this looks good, thanks.



================
Comment at: clang/include/clang/Serialization/ASTReader.h:555
+  /// Key used to identify LifetimeExtendedTemporaryDecl for merging.
+  /// containg the lifetime-extending declaration and the mangling number.
+  using LETemporaryKey = std::pair<Decl *, unsigned>;
----------------
Typos:

"[...] merging.
containg the [...]"

->

"[...] merging,
containing the [...]"


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:1349-1350
+  }
+  OS << " subexpr";
+  dumpPointer(D);
+}
----------------
We shouldn't need this: the address of the declaration is dumped anyway by the infrastructure. (If you meant to dump the subexpression, I don't think that's what this does.)

Traversing from the `LifetimeExtendedTemporaryDecl` to its subexpression for dumping purposes should be done by `ASTNodeTraverser` (in `include/clang/AST/ASTNodeTraverser.h`).


================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:2566
 
+  Decl* RealDecl = static_cast<T *>(D);
+
----------------
`*` on the right, please.


================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:2576
+
+  if (auto *LETDecl = dyn_cast<LifetimeExtendedTemporaryDecl>(RealDecl)) {
+    LETDecl->dump();
----------------
This case is essentially entirely different from the primary implementation of `mergeMergeable`. Consider adding an overload or explicit specialization for the `LifetimeExtendedTemporaryDecl` case instead; we don't need the branch on the `dyn_cast` result below, and we don't need the `allowODRLikeMergeInC` check above (because lifetime-extended temporaries only exist in C++).


================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:2577
+  if (auto *LETDecl = dyn_cast<LifetimeExtendedTemporaryDecl>(RealDecl)) {
+    LETDecl->dump();
+    auto LookupResult =
----------------
We shouldn't be dumping from here :)


================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:2579
+    auto LookupResult =
+        Reader.LETemporaryForMerging.FindAndConstruct(std::make_pair(
+            LETDecl->getExtendingDecl(), LETDecl->getManglingNumber()));
----------------
Prefer the normal container interface here -- `insert` or `operator[]` -- rather than the nonstandard extension `FindAndConstruct`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D70190