[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:
> > > 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?
> Given `makeVisitor` takes the parameters by reference, not value (right?) I don't think there's any scope for an implementation to produce answers other than the ones tested here - (1 move, 0 copies, 1 destruction). If the parameter was passed by value, I think /maybe/ there would be room for implementation variation.
Right, I was missing a few things :^) I think I now understand where the move occurs and why it is moved from exactly once. I'll remove the comments when I make the other changes you suggested!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100670