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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
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);
----------------
dblaikie wrote:
> scott.linder wrote:
> > dblaikie wrote:
> > > 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)
> > Fair point about the ambiguity, but yes I was angling at the `Functor` temporary
> > 
> > I tried to get some practical answer by feeding something similar into a C++14 compiler and a C++17 compiler and asking both to not elide copies. Note that there is no call to the move constructor `Foo(Foo&&)` in the listing from the C++17 compiler https://godbolt.org/z/aMdbYT51h
> > 
> > I'm not clear why there is a 1 move constructor call before the lexical block containing this comment ends, as without the -fno-elide-constructors flag the C++14 compiler doesn't move or copy anything in the godbolt example
> I'm not following how this example (the godbolt link) is analogous to the code being tested here.
> 
> In the code under test, it's more equivalent to this (ignoring moves, just focussing on copies):
> ```
> struct t1 {
>   t1();
>   t1(const t1&);
> };
> struct t2 {
>   t1 m1;
>   t2(const t1& m1) : m1(m1) { }
>   t2(const t2&);
> };
> t2 f1(const t1& v1) {
>   return t2(v1);
> }
> void f2() {
>   t2 v2 = f1(t1());
> }
> ```
> And in this code the copy from the m1 parameter to the m1 member (there was nothing like this in the godbolt link - it's an implementation detail of the `g` function in your example) must be a copy (or a move if we were doing all the move things) - everything else can be elided (or is required to be elided in certain versions of C++). If `f1` took its parameter by value, similarly - that could be elided (since `t1()` is a temporary, so the compiler can construct that temporary right in the by-value parameter location and wouldn't need to copy/move it into position)
Ah, right, the copy/move is occurring inside of `makeVisitor`. Then I think my `TODO` could be "C++14 compilers are not required to elide this temporary `Functor`, so Moves could also be 2u here" but if the existing tests are doing OK then I imagine every compiler is eliding it anyway?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100670