[PATCH] D100670: [ADT] Add makeVisitor to STLExtras.h

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/unittests/ADT/STLExtrasTest.cpp:187-189
+TEST(STLExtrasTest, EnumerateLifetimeSemanticsRValue) {
+  // With an rvalue, it should not be destroyed until the end of the scope.
+  unsigned Moves{}, Copies{}, Destructs{};
----------------
There's a lot of, what seem to be, unrelated test changes in this patch - if they are needed (to account for refactoring of the Range type, for instance) - perhaps they could go in a separate patch?

(though on this particular change - I don't think it's an improvement (to change "X y = 0; Y z = 0; .." to "X y{}, z{}") - I'd say leave it as-is, change the type if needed, and the order of declarations if that helps makes things more consistent.


================
Comment at: llvm/unittests/ADT/STLExtrasTest.cpp:844-845
+    (void)V;
+    // FIXME: Can a C++14 compiler elide this temporary entirely? As of C++20
+    // this will be required (i.e. no temporary may exist in this case.)
+    EXPECT_EQ(1u, Moves);
----------------
Which temporary? (perhaps the comment could be more descriptive/clear) The "Functor" temporary that is eventually moved into the visitor object? I don't think that can be elided (because at the time it's created, in the caller's stack, the final location inside the visitor object is not known - so it can't be constructed in-place)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100670