This is an archive of the discontinued LLVM Phabricator instance.

Differential D100670

[ADT] Add makeVisitor to STLExtras.h
ClosedPublic

Authored by scott.linder on Apr 16 2021, 11:32 AM.

Details

Reviewers
dblaikie
mehdi_amini
bakhtiyarneyman
Commits
rG6d6f35eb7b92: [ADT] Add makeVisitor to STLExtras.h
rG14a8aa615597: [ADT] Add makeVisitor to STLExtras.h
Summary

Adds a utility to combine multiple Callables into a single Callable.
This is useful to make constructing a visitor for std::visit-like
functions more natural; functions like this will be added in future
patches.

Intended to supercede https://reviews.llvm.org/D99560 by
perfectly-forwarding the combined Callables.

Diff Detail

Event Timeline

scott.linder created this revision.Apr 16 2021, 11:32 AM
Herald added a subscriber: dexonsmith. · View Herald TranscriptApr 16 2021, 11:32 AM
scott.linder requested review of this revision.Apr 16 2021, 11:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2021, 11:32 AM
Herald added a subscriber: llvm-commits. · View Herald Transcript
scott.linder added a parent revision: D100669: [ADT] Add llvm::remove_cvref and llvm::remove_cvref_t.Apr 16 2021, 11:34 AM
scott.linder mentioned this in D99560: Utility to construct visitors from lambdas..Apr 16 2021, 11:43 AM
scott.linder added reviewers: dblaikie, mehdi_amini, bakhtiyarneyman.Apr 16 2021, 11:46 AM
Harbormaster completed remote builds in B99223: Diff 338180.Apr 16 2021, 2:05 PM
dblaikie added a comment.Apr 18 2021, 3:13 PM

The intent is that this supersedes/is an alternative to https://reviews.llvm.org/D99560 ? (probably worth mentioning that in the patch/review description then)

scott.linder updated this revision to Diff 346005.May 17 2021, 3:38 PM
scott.linder edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B104914: Diff 346005.May 17 2021, 4:53 PM
dblaikie added inline comments.May 21 2021, 12:48 PM
llvm/unittests/ADT/STLExtrasTest.cpp
210–211

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.

859–860

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)

scott.linder added inline comments.May 21 2021, 2:12 PM
llvm/unittests/ADT/STLExtrasTest.cpp
859–860

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

dblaikie added inline comments.May 21 2021, 2:50 PM
llvm/unittests/ADT/STLExtrasTest.cpp
859–860

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)

scott.linder added inline comments.May 24 2021, 6:57 AM
llvm/unittests/ADT/STLExtrasTest.cpp
859–860

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?

scott.linder added a child revision: D98477: [ADT] Add IntrusiveVariant, VariantTraits, and new STLForwardCompat.May 24 2021, 9:43 AM
dblaikie added inline comments.May 24 2021, 4:59 PM
llvm/unittests/ADT/STLExtrasTest.cpp
859–860

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.

scott.linder added inline comments.May 26 2021, 1:06 PM
llvm/unittests/ADT/STLExtrasTest.cpp
859–860

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!

scott.linder updated this revision to Diff 348087.May 26 2021, 1:57 PM

Rebase and remove incorrect comments

Harbormaster completed remote builds in B106376: Diff 348087.May 26 2021, 1:58 PM
scott.linder added a parent revision: D103206: [ADT] Refactor enumerate unit tests.May 26 2021, 2:09 PM
whisperity added a subscriber: whisperity.Jun 3 2021, 2:17 AM
scott.linder added a comment.Jun 14 2021, 4:25 PM

Ping

dblaikie accepted this revision.Jun 14 2021, 7:00 PM

Looks good, thanks!

This revision is now accepted and ready to land.Jun 14 2021, 7:00 PM
scott.linder updated this revision to Diff 354980.Jun 28 2021, 12:09 PM

rebase

This revision was landed with ongoing or failed builds.Jun 28 2021, 12:36 PM
Closed by commit rG14a8aa615597: [ADT] Add makeVisitor to STLExtras.h (authored by scott.linder). · Explain Why
This revision was automatically updated to reflect the committed changes.
scott.linder added a commit: rG14a8aa615597: [ADT] Add makeVisitor to STLExtras.h.
scott.linder added a reverting change: rG61242c0addb1: Revert "[ADT] Add makeVisitor to STLExtras.h".Jun 28 2021, 12:52 PM
scott.linder added a commit: rG6d6f35eb7b92: [ADT] Add makeVisitor to STLExtras.h.Jun 28 2021, 1:07 PM
Harbormaster completed remote builds in B111343: Diff 354980.Jun 28 2021, 1:35 PM
stella.stamenova added a subscriber: stella.stamenova.Jul 1 2021, 9:42 AM

A couple of the new test cases are consistently failing on Windows when building Debug. They do pass in Release

2021-07-01T09:35:59.0150364Z -- Testing: 76257 tests, 32 workers --
2021-07-01T10:01:33.7697594Z Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80..
2021-07-01T10:01:33.7699468Z FAIL: LLVM-Unit :: ADT/./ADTTests.exe/STLExtrasTest.MakeVisitorDefaultCase (67224 of 76257)
2021-07-01T10:01:33.7701921Z ******************** TEST 'LLVM-Unit :: ADT/./ADTTests.exe/STLExtrasTest.MakeVisitorDefaultCase' FAILED ********************
2021-07-01T10:01:33.7705176Z Script:
2021-07-01T10:01:33.7707482Z --
2021-07-01T10:01:33.7708449Z D:\a\_work\1\b\llvm\Debug\unittests\ADT\.\ADTTests.exe --gtest_filter=STLExtrasTest.MakeVisitorDefaultCase
2021-07-01T10:01:33.7710536Z --
2021-07-01T10:01:33.7711290Z Note: Google Test filter = STLExtrasTest.MakeVisitorDefaultCase
2021-07-01T10:01:33.7711592Z 
2021-07-01T10:01:33.7712142Z [==========] Running 1 test from 1 test suite.
2021-07-01T10:01:33.7712386Z 
2021-07-01T10:01:33.7712823Z [----------] Global test environment set-up.
2021-07-01T10:01:33.7713592Z 
2021-07-01T10:01:33.7717503Z [----------] 1 test from STLExtrasTest
2021-07-01T10:01:33.7718505Z 
2021-07-01T10:01:33.7720213Z [ RUN      ] STLExtrasTest.MakeVisitorDefaultCase
2021-07-01T10:01:33.7720490Z 
2021-07-01T10:01:33.7721127Z D:\a\_work\1\s\llvm\unittests\ADT\STLExtrasTest.cpp(799): error: Expected equality of these values:
2021-07-01T10:01:33.7721574Z 
2021-07-01T10:01:33.7721953Z   Visitor(2.)
2021-07-01T10:01:33.7722097Z 
2021-07-01T10:01:33.7722524Z     Which is: 00007FF724E66A30
2021-07-01T10:01:33.7722710Z 
2021-07-01T10:01:33.7723131Z   "unhandled type"
2021-07-01T10:01:33.7723283Z 
2021-07-01T10:01:33.7723697Z     Which is: 00007FF724E53048
2021-07-01T10:01:33.7723881Z 
2021-07-01T10:01:33.7724532Z D:\a\_work\1\s\llvm\unittests\ADT\STLExtrasTest.cpp(800): error: Expected equality of these values:
2021-07-01T10:01:33.7724903Z 
2021-07-01T10:01:33.7725284Z   Visitor(Visitor)
2021-07-01T10:01:33.7725438Z 
2021-07-01T10:01:33.7725887Z     Which is: 00007FF724E66A30
2021-07-01T10:01:33.7726076Z 
2021-07-01T10:01:33.7726464Z   "unhandled type"
2021-07-01T10:01:33.7726655Z 
2021-07-01T10:01:33.7727072Z     Which is: 00007FF724E530C8
2021-07-01T10:01:33.7727802Z 
2021-07-01T10:01:33.7728452Z D:\a\_work\1\s\llvm\unittests\ADT\STLExtrasTest.cpp(808): error: Expected equality of these values:
2021-07-01T10:01:33.7729535Z 
2021-07-01T10:01:33.7730317Z   Visitor(2.)
2021-07-01T10:01:33.7730682Z 
2021-07-01T10:01:33.7731329Z     Which is: 00007FF724E66A30
2021-07-01T10:01:33.7731592Z 
2021-07-01T10:01:33.7732027Z   "unhandled type"
2021-07-01T10:01:33.7732182Z 
2021-07-01T10:01:33.7732641Z     Which is: 00007FF724E531E0
2021-07-01T10:01:33.7732826Z 
2021-07-01T10:01:33.7733469Z D:\a\_work\1\s\llvm\unittests\ADT\STLExtrasTest.cpp(809): error: Expected equality of these values:
2021-07-01T10:01:33.7733845Z 
2021-07-01T10:01:33.7734228Z   Visitor(Visitor)
2021-07-01T10:01:33.7734407Z 
2021-07-01T10:01:33.7735018Z     Which is: 00007FF724E66A30
2021-07-01T10:01:33.7735513Z 
2021-07-01T10:01:33.7736034Z   "unhandled type"
2021-07-01T10:01:33.7736306Z 
2021-07-01T10:01:33.7736778Z     Which is: 00007FF724E53258
2021-07-01T10:01:33.7737025Z 
2021-07-01T10:01:33.7739580Z [  FAILED  ] STLExtrasTest.MakeVisitorDefaultCase (2 ms)
2021-07-01T10:01:33.7739859Z 
2021-07-01T10:01:33.7740503Z [----------] 1 test from STLExtrasTest (2 ms total)
2021-07-01T10:01:33.7740744Z 
2021-07-01T10:01:33.7740854Z 
2021-07-01T10:01:33.7740961Z 
2021-07-01T10:01:33.7741379Z [----------] Global test environment tear-down
2021-07-01T10:01:33.7741628Z 
2021-07-01T10:01:33.7742090Z [==========] 1 test from 1 test suite ran. (2 ms total)
2021-07-01T10:01:33.7742296Z 
2021-07-01T10:01:33.7742663Z [  PASSED  ] 0 tests.
2021-07-01T10:01:33.7742832Z 
2021-07-01T10:01:33.7743244Z [  FAILED  ] 1 test, listed below:
2021-07-01T10:01:33.7743439Z 
2021-07-01T10:01:33.7743884Z [  FAILED  ] STLExtrasTest.MakeVisitorDefaultCase
2021-07-01T10:01:33.7744073Z 
2021-07-01T10:01:33.7744183Z 
2021-07-01T10:01:33.7744290Z 
2021-07-01T10:01:33.7744639Z  1 FAILED TEST
2021-07-01T10:01:33.7744812Z 
2021-07-01T10:01:33.7744922Z 
2021-07-01T10:01:33.7745284Z ********************
2021-07-01T10:01:33.7903181Z Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80..
2021-07-01T10:01:33.8009590Z FAIL: LLVM-Unit :: ADT/./ADTTests.exe/STLExtrasTest.MakeVisitorTwoCallables (67229 of 76257)
2021-07-01T10:01:33.8011983Z ******************** TEST 'LLVM-Unit :: ADT/./ADTTests.exe/STLExtrasTest.MakeVisitorTwoCallables' FAILED ********************
2021-07-01T10:01:33.8029530Z Script:
2021-07-01T10:01:33.8061592Z --
2021-07-01T10:01:33.8062299Z D:\a\_work\1\b\llvm\Debug\unittests\ADT\.\ADTTests.exe --gtest_filter=STLExtrasTest.MakeVisitorTwoCallables
2021-07-01T10:01:33.8062912Z --
2021-07-01T10:01:33.8063391Z Note: Google Test filter = STLExtrasTest.MakeVisitorTwoCallables
2021-07-01T10:01:33.8063636Z 
2021-07-01T10:01:33.8064103Z [==========] Running 1 test from 1 test suite.
2021-07-01T10:01:33.8064308Z 
2021-07-01T10:01:33.8064738Z [----------] Global test environment set-up.
2021-07-01T10:01:33.8065209Z 
2021-07-01T10:01:33.8065628Z [----------] 1 test from STLExtrasTest
2021-07-01T10:01:33.8065845Z 
2021-07-01T10:01:33.8066280Z [ RUN      ] STLExtrasTest.MakeVisitorTwoCallables
2021-07-01T10:01:33.8066511Z 
2021-07-01T10:01:33.8067118Z D:\a\_work\1\s\llvm\unittests\ADT\STLExtrasTest.cpp(781): error: Expected equality of these values:
2021-07-01T10:01:33.8067483Z 
2021-07-01T10:01:33.8068073Z   Visitor(42)
2021-07-01T10:01:33.8068229Z 
2021-07-01T10:01:33.8068651Z     Which is: 00007FF724E66994
2021-07-01T10:01:33.8068833Z 
2021-07-01T10:01:33.8069212Z   "int"
2021-07-01T10:01:33.8069387Z 
2021-07-01T10:01:33.8069800Z     Which is: 00007FF724E51D5C
2021-07-01T10:01:33.8070133Z 
2021-07-01T10:01:33.8070722Z D:\a\_work\1\s\llvm\unittests\ADT\STLExtrasTest.cpp(782): error: Expected equality of these values:
2021-07-01T10:01:33.8071077Z 
2021-07-01T10:01:33.8071466Z   Visitor("foo")
2021-07-01T10:01:33.8071646Z 
2021-07-01T10:01:33.8072206Z     Which is: 00007FF724E66998
2021-07-01T10:01:33.8072395Z 
2021-07-01T10:01:33.8072764Z   "str"
2021-07-01T10:01:33.8073016Z 
2021-07-01T10:01:33.8073418Z     Which is: 00007FF724E51D94
2021-07-01T10:01:33.8073593Z 
2021-07-01T10:01:33.8074050Z [  FAILED  ] STLExtrasTest.MakeVisitorTwoCallables (0 ms)
2021-07-01T10:01:33.8074268Z 
2021-07-01T10:01:33.8074693Z [----------] 1 test from STLExtrasTest (0 ms total)
2021-07-01T10:01:33.8074919Z 
2021-07-01T10:01:33.8075033Z 
2021-07-01T10:01:33.8075144Z 
2021-07-01T10:01:33.8075553Z [----------] Global test environment tear-down
2021-07-01T10:01:33.8075771Z 
2021-07-01T10:01:33.8076204Z [==========] 1 test from 1 test suite ran. (2 ms total)
2021-07-01T10:01:33.8076417Z 
2021-07-01T10:01:33.8076786Z [  PASSED  ] 0 tests.
2021-07-01T10:01:33.8076958Z 
2021-07-01T10:01:33.8077371Z [  FAILED  ] 1 test, listed below:
2021-07-01T10:01:33.8077568Z 
2021-07-01T10:01:33.8078005Z [  FAILED  ] STLExtrasTest.MakeVisitorTwoCallables
2021-07-01T10:01:33.8078209Z 
2021-07-01T10:01:33.8078330Z 
2021-07-01T10:01:33.8078459Z 
2021-07-01T10:01:33.8078817Z  1 FAILED TEST
2021-07-01T10:01:33.8080335Z 
2021-07-01T10:01:33.8080521Z 
2021-07-01T10:01:33.8081113Z ********************
2021-07-01T10:02:59.3969341Z Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
2021-07-01T10:02:59.7189730Z 
2021-07-01T10:02:59.7190464Z 1 warning(s) in tests
2021-07-01T10:02:59.7711389Z ********************
2021-07-01T10:02:59.7713124Z Failed Tests (2):
2021-07-01T10:02:59.7713692Z   LLVM-Unit :: ADT/./ADTTests.exe/STLExtrasTest.MakeVisitorDefaultCase
2021-07-01T10:02:59.7714235Z   LLVM-Unit :: ADT/./ADTTests.exe/STLExtrasTest.MakeVisitorTwoCallables
2021-07-01T10:02:59.7714448Z 
2021-07-01T10:02:59.7714545Z 
2021-07-01T10:02:59.7714906Z Testing Time: 1646.03s
2021-07-01T10:02:59.7715299Z   Skipped          :    39
2021-07-01T10:02:59.7715766Z   Unsupported      : 21785
2021-07-01T10:02:59.7716168Z   Passed           : 54327
2021-07-01T10:02:59.7716570Z   Expectedly Failed:   104
2021-07-01T10:02:59.7717012Z   Failed           :     2
scott.linder added a comment.Jul 1 2021, 11:06 AM
In D100670#2853327, @stella.stamenova wrote:

A couple of the new test cases are consistently failing on Windows when building Debug. They do pass in Release

2021-07-01T09:35:59.0150364Z -- Testing: 76257 tests, 32 workers --
2021-07-01T10:01:33.7697594Z Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80..
2021-07-01T10:01:33.7699468Z FAIL: LLVM-Unit :: ADT/./ADTTests.exe/STLExtrasTest.MakeVisitorDefaultCase (67224 of 76257)
2021-07-01T10:01:33.7701921Z ******************** TEST 'LLVM-Unit :: ADT/./ADTTests.exe/STLExtrasTest.MakeVisitorDefaultCase' FAILED ********************
2021-07-01T10:01:33.7705176Z Script:
2021-07-01T10:01:33.7707482Z --
2021-07-01T10:01:33.7708449Z D:\a\_work\1\b\llvm\Debug\unittests\ADT\.\ADTTests.exe --gtest_filter=STLExtrasTest.MakeVisitorDefaultCase
2021-07-01T10:01:33.7710536Z --
2021-07-01T10:01:33.7711290Z Note: Google Test filter = STLExtrasTest.MakeVisitorDefaultCase
2021-07-01T10:01:33.7711592Z 
2021-07-01T10:01:33.7712142Z [==========] Running 1 test from 1 test suite.
2021-07-01T10:01:33.7712386Z 
2021-07-01T10:01:33.7712823Z [----------] Global test environment set-up.
2021-07-01T10:01:33.7713592Z 
2021-07-01T10:01:33.7717503Z [----------] 1 test from STLExtrasTest
2021-07-01T10:01:33.7718505Z 
2021-07-01T10:01:33.7720213Z [ RUN      ] STLExtrasTest.MakeVisitorDefaultCase
2021-07-01T10:01:33.7720490Z 
2021-07-01T10:01:33.7721127Z D:\a\_work\1\s\llvm\unittests\ADT\STLExtrasTest.cpp(799): error: Expected equality of these values:
2021-07-01T10:01:33.7721574Z 
2021-07-01T10:01:33.7721953Z   Visitor(2.)
2021-07-01T10:01:33.7722097Z 
2021-07-01T10:01:33.7722524Z     Which is: 00007FF724E66A30
2021-07-01T10:01:33.7722710Z 
2021-07-01T10:01:33.7723131Z   "unhandled type"
2021-07-01T10:01:33.7723283Z 
2021-07-01T10:01:33.7723697Z     Which is: 00007FF724E53048
2021-07-01T10:01:33.7723881Z 
2021-07-01T10:01:33.7724532Z D:\a\_work\1\s\llvm\unittests\ADT\STLExtrasTest.cpp(800): error: Expected equality of these values:
2021-07-01T10:01:33.7724903Z 
2021-07-01T10:01:33.7725284Z   Visitor(Visitor)
2021-07-01T10:01:33.7725438Z 
2021-07-01T10:01:33.7725887Z     Which is: 00007FF724E66A30
2021-07-01T10:01:33.7726076Z 
2021-07-01T10:01:33.7726464Z   "unhandled type"
2021-07-01T10:01:33.7726655Z 
2021-07-01T10:01:33.7727072Z     Which is: 00007FF724E530C8
2021-07-01T10:01:33.7727802Z 
2021-07-01T10:01:33.7728452Z D:\a\_work\1\s\llvm\unittests\ADT\STLExtrasTest.cpp(808): error: Expected equality of these values:
2021-07-01T10:01:33.7729535Z 
2021-07-01T10:01:33.7730317Z   Visitor(2.)
2021-07-01T10:01:33.7730682Z 
2021-07-01T10:01:33.7731329Z     Which is: 00007FF724E66A30
2021-07-01T10:01:33.7731592Z 
2021-07-01T10:01:33.7732027Z   "unhandled type"
2021-07-01T10:01:33.7732182Z 
2021-07-01T10:01:33.7732641Z     Which is: 00007FF724E531E0
2021-07-01T10:01:33.7732826Z 
2021-07-01T10:01:33.7733469Z D:\a\_work\1\s\llvm\unittests\ADT\STLExtrasTest.cpp(809): error: Expected equality of these values:
2021-07-01T10:01:33.7733845Z 
2021-07-01T10:01:33.7734228Z   Visitor(Visitor)
2021-07-01T10:01:33.7734407Z 
2021-07-01T10:01:33.7735018Z     Which is: 00007FF724E66A30
2021-07-01T10:01:33.7735513Z 
2021-07-01T10:01:33.7736034Z   "unhandled type"
2021-07-01T10:01:33.7736306Z 
2021-07-01T10:01:33.7736778Z     Which is: 00007FF724E53258
2021-07-01T10:01:33.7737025Z 
2021-07-01T10:01:33.7739580Z [  FAILED  ] STLExtrasTest.MakeVisitorDefaultCase (2 ms)
2021-07-01T10:01:33.7739859Z 
2021-07-01T10:01:33.7740503Z [----------] 1 test from STLExtrasTest (2 ms total)
2021-07-01T10:01:33.7740744Z 
2021-07-01T10:01:33.7740854Z 
2021-07-01T10:01:33.7740961Z 
2021-07-01T10:01:33.7741379Z [----------] Global test environment tear-down
2021-07-01T10:01:33.7741628Z 
2021-07-01T10:01:33.7742090Z [==========] 1 test from 1 test suite ran. (2 ms total)
2021-07-01T10:01:33.7742296Z 
2021-07-01T10:01:33.7742663Z [  PASSED  ] 0 tests.
2021-07-01T10:01:33.7742832Z 
2021-07-01T10:01:33.7743244Z [  FAILED  ] 1 test, listed below:
2021-07-01T10:01:33.7743439Z 
2021-07-01T10:01:33.7743884Z [  FAILED  ] STLExtrasTest.MakeVisitorDefaultCase
2021-07-01T10:01:33.7744073Z 
2021-07-01T10:01:33.7744183Z 
2021-07-01T10:01:33.7744290Z 
2021-07-01T10:01:33.7744639Z  1 FAILED TEST
2021-07-01T10:01:33.7744812Z 
2021-07-01T10:01:33.7744922Z 
2021-07-01T10:01:33.7745284Z ********************
2021-07-01T10:01:33.7903181Z Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80..
2021-07-01T10:01:33.8009590Z FAIL: LLVM-Unit :: ADT/./ADTTests.exe/STLExtrasTest.MakeVisitorTwoCallables (67229 of 76257)
2021-07-01T10:01:33.8011983Z ******************** TEST 'LLVM-Unit :: ADT/./ADTTests.exe/STLExtrasTest.MakeVisitorTwoCallables' FAILED ********************
2021-07-01T10:01:33.8029530Z Script:
2021-07-01T10:01:33.8061592Z --
2021-07-01T10:01:33.8062299Z D:\a\_work\1\b\llvm\Debug\unittests\ADT\.\ADTTests.exe --gtest_filter=STLExtrasTest.MakeVisitorTwoCallables
2021-07-01T10:01:33.8062912Z --
2021-07-01T10:01:33.8063391Z Note: Google Test filter = STLExtrasTest.MakeVisitorTwoCallables
2021-07-01T10:01:33.8063636Z 
2021-07-01T10:01:33.8064103Z [==========] Running 1 test from 1 test suite.
2021-07-01T10:01:33.8064308Z 
2021-07-01T10:01:33.8064738Z [----------] Global test environment set-up.
2021-07-01T10:01:33.8065209Z 
2021-07-01T10:01:33.8065628Z [----------] 1 test from STLExtrasTest
2021-07-01T10:01:33.8065845Z 
2021-07-01T10:01:33.8066280Z [ RUN      ] STLExtrasTest.MakeVisitorTwoCallables
2021-07-01T10:01:33.8066511Z 
2021-07-01T10:01:33.8067118Z D:\a\_work\1\s\llvm\unittests\ADT\STLExtrasTest.cpp(781): error: Expected equality of these values:
2021-07-01T10:01:33.8067483Z 
2021-07-01T10:01:33.8068073Z   Visitor(42)
2021-07-01T10:01:33.8068229Z 
2021-07-01T10:01:33.8068651Z     Which is: 00007FF724E66994
2021-07-01T10:01:33.8068833Z 
2021-07-01T10:01:33.8069212Z   "int"
2021-07-01T10:01:33.8069387Z 
2021-07-01T10:01:33.8069800Z     Which is: 00007FF724E51D5C
2021-07-01T10:01:33.8070133Z 
2021-07-01T10:01:33.8070722Z D:\a\_work\1\s\llvm\unittests\ADT\STLExtrasTest.cpp(782): error: Expected equality of these values:
2021-07-01T10:01:33.8071077Z 
2021-07-01T10:01:33.8071466Z   Visitor("foo")
2021-07-01T10:01:33.8071646Z 
2021-07-01T10:01:33.8072206Z     Which is: 00007FF724E66998
2021-07-01T10:01:33.8072395Z 
2021-07-01T10:01:33.8072764Z   "str"
2021-07-01T10:01:33.8073016Z 
2021-07-01T10:01:33.8073418Z     Which is: 00007FF724E51D94
2021-07-01T10:01:33.8073593Z 
2021-07-01T10:01:33.8074050Z [  FAILED  ] STLExtrasTest.MakeVisitorTwoCallables (0 ms)
2021-07-01T10:01:33.8074268Z 
2021-07-01T10:01:33.8074693Z [----------] 1 test from STLExtrasTest (0 ms total)
2021-07-01T10:01:33.8074919Z 
2021-07-01T10:01:33.8075033Z 
2021-07-01T10:01:33.8075144Z 
2021-07-01T10:01:33.8075553Z [----------] Global test environment tear-down
2021-07-01T10:01:33.8075771Z 
2021-07-01T10:01:33.8076204Z [==========] 1 test from 1 test suite ran. (2 ms total)
2021-07-01T10:01:33.8076417Z 
2021-07-01T10:01:33.8076786Z [  PASSED  ] 0 tests.
2021-07-01T10:01:33.8076958Z 
2021-07-01T10:01:33.8077371Z [  FAILED  ] 1 test, listed below:
2021-07-01T10:01:33.8077568Z 
2021-07-01T10:01:33.8078005Z [  FAILED  ] STLExtrasTest.MakeVisitorTwoCallables
2021-07-01T10:01:33.8078209Z 
2021-07-01T10:01:33.8078330Z 
2021-07-01T10:01:33.8078459Z 
2021-07-01T10:01:33.8078817Z  1 FAILED TEST
2021-07-01T10:01:33.8080335Z 
2021-07-01T10:01:33.8080521Z 
2021-07-01T10:01:33.8081113Z ********************
2021-07-01T10:02:59.3969341Z Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
2021-07-01T10:02:59.7189730Z 
2021-07-01T10:02:59.7190464Z 1 warning(s) in tests
2021-07-01T10:02:59.7711389Z ********************
2021-07-01T10:02:59.7713124Z Failed Tests (2):
2021-07-01T10:02:59.7713692Z   LLVM-Unit :: ADT/./ADTTests.exe/STLExtrasTest.MakeVisitorDefaultCase
2021-07-01T10:02:59.7714235Z   LLVM-Unit :: ADT/./ADTTests.exe/STLExtrasTest.MakeVisitorTwoCallables
2021-07-01T10:02:59.7714448Z 
2021-07-01T10:02:59.7714545Z 
2021-07-01T10:02:59.7714906Z Testing Time: 1646.03s
2021-07-01T10:02:59.7715299Z   Skipped          :    39
2021-07-01T10:02:59.7715766Z   Unsupported      : 21785
2021-07-01T10:02:59.7716168Z   Passed           : 54327
2021-07-01T10:02:59.7716570Z   Expectedly Failed:   104
2021-07-01T10:02:59.7717012Z   Failed           :     2

Looking at this now, if I can't root cause quickly I will revert again

scott.linder added inline comments.Jul 1 2021, 11:16 AM
llvm/unittests/ADT/STLExtrasTest.cpp
834

(Accidentally posted this on the commit itself first, reposting here)

Ah, I believe my mistake was assuming EXPECT_EQ treated char * as a string, rather than like any other pointer. I didn't intend to compare the pointer values here, but it accidentally succeeds when the compiler merges the storage for both string literals.

I'll have a patch up shortly to fix the bug.

scott.linder added a comment.Jul 1 2021, 11:26 AM

The fix should be in 83887df15597990308e9903d0480fa7676d772a1, let me know if there are still failures, and sorry for the noise!

Revision Contents

PathSize
llvm/
include/
llvm/
ADT/
52 lines
unittests/
ADT/
112 lines

Diff 348087

llvm/include/llvm/ADT/STLExtras.h

Show First 20 Lines • Show All 1,338 Lines • ▼ Show 20 Linesstruct on_first {
} }
}; };
/// Utility type to build an inheritance chain that makes it easy to rank /// Utility type to build an inheritance chain that makes it easy to rank
/// overload candidates. /// overload candidates.
template <int N> struct rank : rank<N - 1> {}; template <int N> struct rank : rank<N - 1> {};
template <> struct rank<0> {}; template <> struct rank<0> {};
namespace detail {
template <typename... Ts> struct Visitor;
template <typename HeadT, typename... TailTs>
struct Visitor<HeadT, TailTs...> : remove_cvref_t<HeadT>, Visitor<TailTs...> {
explicit constexpr Visitor(HeadT &&Head, TailTs &&...Tail)
: remove_cvref_t<HeadT>(std::forward<HeadT>(Head)),
Visitor<TailTs...>(std::forward<TailTs>(Tail)...) {}
using remove_cvref_t<HeadT>::operator();
using Visitor<TailTs...>::operator();
};
template <typename HeadT> struct Visitor<HeadT> : remove_cvref_t<HeadT> {
explicit constexpr Visitor(HeadT &&Head)
: remove_cvref_t<HeadT>(std::forward<HeadT>(Head)) {}
using remove_cvref_t<HeadT>::operator();
};
} // namespace detail
/// Returns an opaquely-typed Callable object whose operator() overload set is
/// the sum of the operator() overload sets of each CallableT in CallableTs.
///
/// The type of the returned object derives from each CallableT in CallableTs.
/// The returned object is constructed by invoking the appropriate copy or move
/// constructor of each CallableT, as selected by overload resolution on the
/// corresponding argument to makeVisitor.
///
/// Example:
///
/// \code
/// auto visitor = makeVisitor([](auto) { return "unhandled type"; },
/// [](int i) { return "int"; },
/// [](std::string s) { return "str"; });
/// auto a = visitor(42); // `a` is now "int".
/// auto b = visitor("foo"); // `b` is now "str".
/// auto c = visitor(3.14f); // `c` is now "unhandled type".
/// \endcode
///
/// Example of making a visitor with a lambda which captures a move-only type:
///
/// \code
/// std::unique_ptr<FooHandler> FH = /* ... */;
/// auto visitor = makeVisitor(
/// [FH{std::move(FH)}](Foo F) { return FH->handle(F); },
/// [](int i) { return i; },
/// [](std::string s) { return atoi(s); });
/// \endcode
template <typename... CallableTs>
constexpr decltype(auto) makeVisitor(CallableTs &&...Callables) {
return detail::Visitor<CallableTs...>(std::forward<CallableTs>(Callables)...);
}
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
// Extra additions for arrays // Extra additions for arrays
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
// We have a copy here so that LLVM behaves the same when using different // We have a copy here so that LLVM behaves the same when using different
// standard libraries. // standard libraries.
template <class Iterator, class RNG> template <class Iterator, class RNG>
void shuffle(Iterator first, Iterator last, RNG &&g) { void shuffle(Iterator first, Iterator last, RNG &&g) {
▲ Show 20 Lines • Show All 710 Lines • Show Last 20 Lines

llvm/unittests/ADT/STLExtrasTest.cpp

Show First 20 Lines • Show All 201 Lines • ▼ Show 20 Lines int Destructors = 0;
EXPECT_EQ(1, Moves); EXPECT_EQ(1, Moves);
EXPECT_EQ(1, Destructors); EXPECT_EQ(1, Destructors);
} }
EXPECT_EQ(0, Copies); EXPECT_EQ(0, Copies);
EXPECT_EQ(1, Moves); EXPECT_EQ(1, Moves);
EXPECT_EQ(2, Destructors); EXPECT_EQ(2, Destructors);
} }
TEST(STLExtrasTest, EnumerateLifetimeSemanticsLValue) { TEST(STLExtrasTest, EnumerateLifetimeSemanticsLValue) {
// With an lvalue, it should not be destroyed even after the end of the scope. // With an lvalue, it should not be destroyed even after the end of the scope.
dblaikieUnsubmitted
Not Done
ReplyInline Actions

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.

dblaikie: There's a lot of, what seem to be, unrelated test changes in this patch - if they are needed…
// lvalue ranges need be neither copyable nor moveable. // lvalue ranges need be neither copyable nor moveable.
int Copies = 0; int Copies = 0;
int Moves = 0; int Moves = 0;
int Destructors = 0; int Destructors = 0;
{ {
Range<false, false> R(Copies, Moves, Destructors); Range<false, false> R(Copies, Moves, Destructors);
{ {
auto E = enumerate(R); auto E = enumerate(R);
▲ Show 20 Lines • Show All 574 Lines • ▼ Show 20 LinesTEST(STLExtrasTest, TypeAtIndex) {
EXPECT_TRUE( EXPECT_TRUE(
(std::is_same<float, llvm::TypeAtIndex<1, int, float, double>>::value)); (std::is_same<float, llvm::TypeAtIndex<1, int, float, double>>::value));
EXPECT_TRUE( EXPECT_TRUE(
(std::is_same<float, llvm::TypeAtIndex<1, int, float, double>>::value)); (std::is_same<float, llvm::TypeAtIndex<1, int, float, double>>::value));
EXPECT_TRUE( EXPECT_TRUE(
(std::is_same<double, llvm::TypeAtIndex<2, int, float, double>>::value)); (std::is_same<double, llvm::TypeAtIndex<2, int, float, double>>::value));
} }
TEST(STLExtrasTest, MakeVisitorOneCallable) {
auto IdentityLambda = [](auto X) { return X; };
auto IdentityVisitor = makeVisitor(IdentityLambda);
EXPECT_EQ(IdentityLambda(1), IdentityVisitor(1));
EXPECT_EQ(IdentityLambda(2.0f), IdentityVisitor(2.0f));
EXPECT_TRUE((std::is_same<decltype(IdentityLambda(IdentityLambda)),
decltype(IdentityLambda)>::value));
EXPECT_TRUE((std::is_same<decltype(IdentityVisitor(IdentityVisitor)),
decltype(IdentityVisitor)>::value));
}
TEST(STLExtrasTest, MakeVisitorTwoCallables) {
auto Visitor =
makeVisitor([](int) { return "int"; }, [](std::string) { return "str"; });
EXPECT_EQ(Visitor(42), "int");
EXPECT_EQ(Visitor("foo"), "str");
}
TEST(STLExtrasTest, MakeVisitorCallableMultipleOperands) {
auto Second = makeVisitor([](int I, float F) { return F; },
[](float F, int I) { return I; });
EXPECT_EQ(Second(1.f, 1), 1);
EXPECT_EQ(Second(1, 1.f), 1.f);
}
TEST(STLExtrasTest, MakeVisitorDefaultCase) {
{
auto Visitor = makeVisitor([](int I) { return I + 100; },
[](float F) { return F * 2; },
[](auto) { return "unhandled type"; });
EXPECT_EQ(Visitor(24), 124);
EXPECT_EQ(Visitor(2.f), 4.f);
EXPECT_EQ(Visitor(2.), "unhandled type");
scott.linderAuthorUnsubmitted
Done
ReplyInline Actions

(Accidentally posted this on the commit itself first, reposting here)

Ah, I believe my mistake was assuming EXPECT_EQ treated char * as a string, rather than like any other pointer. I didn't intend to compare the pointer values here, but it accidentally succeeds when the compiler merges the storage for both string literals.

I'll have a patch up shortly to fix the bug.

scott.linder: (Accidentally posted this on the commit itself first, reposting here) Ah, I believe my mistake…
EXPECT_EQ(Visitor(Visitor), "unhandled type");
}
{
auto Visitor = makeVisitor([](auto) { return "unhandled type"; },
[](int I) { return I + 100; },
[](float F) { return F * 2; });
EXPECT_EQ(Visitor(24), 124);
EXPECT_EQ(Visitor(2.f), 4.f);
EXPECT_EQ(Visitor(2.), "unhandled type");
EXPECT_EQ(Visitor(Visitor), "unhandled type");
}
}
template <bool Moveable, bool Copyable>
struct Functor : Counted<Moveable, Copyable> {
using Counted<Moveable, Copyable>::Counted;
void operator()() {}
};
TEST(STLExtrasTest, MakeVisitorLifetimeSemanticsPRValue) {
int Copies = 0;
int Moves = 0;
int Destructors = 0;
{
auto V = makeVisitor(Functor<true, false>(Copies, Moves, Destructors));
(void)V;
dblaikieUnsubmitted
Not Done
ReplyInline Actions

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)

dblaikie: Which temporary? (perhaps the comment could be more descriptive/clear) The "Functor" temporary…
scott.linderAuthorUnsubmitted
Done
ReplyInline Actions

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

scott.linder: Fair point about the ambiguity, but yes I was angling at the `Functor` temporary I tried to…
dblaikieUnsubmitted
Not Done
ReplyInline Actions

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)

dblaikie: I'm not following how this example (the godbolt link) is analogous to the code being tested…
scott.linderAuthorUnsubmitted
Done
ReplyInline Actions

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?

scott.linder: Ah, right, the copy/move is occurring inside of `makeVisitor`. Then I think my `TODO` could be…
dblaikieUnsubmitted
Not Done
ReplyInline Actions

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.

dblaikie: Given `makeVisitor` takes the parameters by reference, not value (right?) I don't think there's…
scott.linderAuthorUnsubmitted
Done
ReplyInline Actions

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!

scott.linder: Right, I was missing a few things :^) I think I now understand where the move occurs and why it…
EXPECT_EQ(0, Copies);
EXPECT_EQ(1, Moves);
EXPECT_EQ(1, Destructors);
}
EXPECT_EQ(0, Copies);
EXPECT_EQ(1, Moves);
EXPECT_EQ(2, Destructors);
}
TEST(STLExtrasTest, MakeVisitorLifetimeSemanticsRValue) {
int Copies = 0;
int Moves = 0;
int Destructors = 0;
{
Functor<true, false> F(Copies, Moves, Destructors);
{
auto V = makeVisitor(std::move(F));
(void)V;
EXPECT_EQ(0, Copies);
EXPECT_EQ(1, Moves);
EXPECT_EQ(0, Destructors);
}
EXPECT_EQ(0, Copies);
EXPECT_EQ(1, Moves);
EXPECT_EQ(1, Destructors);
}
EXPECT_EQ(0, Copies);
EXPECT_EQ(1, Moves);
EXPECT_EQ(2, Destructors);
}
TEST(STLExtrasTest, MakeVisitorLifetimeSemanticsLValue) {
int Copies = 0;
int Moves = 0;
int Destructors = 0;
{
Functor<true, true> F(Copies, Moves, Destructors);
{
auto V = makeVisitor(F);
(void)V;
EXPECT_EQ(1, Copies);
EXPECT_EQ(0, Moves);
EXPECT_EQ(0, Destructors);
}
EXPECT_EQ(1, Copies);
EXPECT_EQ(0, Moves);
EXPECT_EQ(1, Destructors);
}
EXPECT_EQ(1, Copies);
EXPECT_EQ(0, Moves);
EXPECT_EQ(2, Destructors);
}
} // namespace } // namespace