feat: reapply fixed version of #3271 #3293

henryiii · 2021-09-22T19:46:15Z

Description

SFINAE was broken, C++ does not perform SFINAE inside classes/structs. If someone wants to implement a better test (one that actually supports keep_alive), happy to take a contribution!

Activating SFINAE shows the new code is broken on some compilers.

Suggested changelog entry:

* Add ``make_value_iterator()``, and fix ``make_key_iterator()`` to return references instead of copies.

henryiii · 2021-09-22T20:14:20Z

@bmerry this should be a good starting point. Fixing this seems to be tripping up the new code, it's trying to make a copy of NonCopyableInt. Could you take a look? I can give you access to the branch if needed.

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DBOOST_ALL_NO_LIB -DPYBIND11_TEST_BOOST -DPYBIND11_TEST_EIGEN -Dpybind11_tests_EXPORTS -I/Users/henryschreiner/git/software/pybind11/include -isystem /usr/local/opt/python@3.9/Frameworks/Python.framework/Versions/3.9/include/python3.9 -isystem /Users/henryschreiner/git/software/pybind11/.nox/tests-3-9/tmp/_deps/eigen-src -isystem /usr/local/include -Os -DNDEBUG -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk -fPIC -fvisibility=hidden -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -Werror -flto=thin -std=gnu++11 -MD -MT tests/CMakeFiles/pybind11_tests.dir/test_sequences_and_iterators.cpp.o -MF tests/CMakeFiles/pybind11_tests.dir/test_sequences_and_iterators.cpp.o.d -o tests/CMakeFiles/pybind11_tests.dir/test_sequences_and_iterators.cpp.o -c /Users/henryschreiner/git/software/pybind11/tests/test_sequences_and_iterators.cpp
In file included from /Users/henryschreiner/git/software/pybind11/tests/test_sequences_and_iterators.cpp:11:
In file included from /Users/henryschreiner/git/software/pybind11/tests/pybind11_tests.h:3:
/Users/henryschreiner/git/software/pybind11/include/pybind11/pybind11.h:1983:16: error: call to deleted constructor of 'pybind11::detail::iterator_key_access<std::__wrap_iter<std::pair<NonCopyableInt, NonCopyableInt> *>, NonCopyableInt>::result_type' (aka 'NonCopyableInt')
        return (*it).first;
               ^~~~~~~~~~~
/Users/henryschreiner/git/software/pybind11/include/pybind11/pybind11.h:2017:24: note: in instantiation of member function 'pybind11::detail::iterator_key_access<std::__wrap_iter<std::pair<NonCopyableInt, NonCopyableInt> *>, NonCopyableInt>::operator()' requested here
                return Access()(s.it);
                       ^
/Users/henryschreiner/git/software/pybind11/include/pybind11/pybind11.h:2051:20: note: in instantiation of function template specialization 'pybind11::detail::make_iterator_impl<pybind11::detail::iterator_key_access<std::__wrap_iter<std::pair<NonCopyableInt, NonCopyableInt> *>, NonCopyableInt>, pybind11::return_value_policy::reference_internal, std::__wrap_iter<std::pair<NonCopyableInt, NonCopyableInt> *>, std::__wrap_iter<std::pair<NonCopyableInt, NonCopyableInt> *>, NonCopyableInt>' requested here
    return detail::make_iterator_impl<
                   ^
/Users/henryschreiner/git/software/pybind11/tests/test_sequences_and_iterators.cpp:351:24: note: in instantiation of function template specialization 'pybind11::make_key_iterator<pybind11::return_value_policy::reference_internal, std::__wrap_iter<std::pair<NonCopyableInt, NonCopyableInt> *>, std::__wrap_iter<std::pair<NonCopyableInt, NonCopyableInt> *>, NonCopyableInt>' requested here
            return py::make_key_iterator(vec.begin(), vec.end());
                       ^
/Users/henryschreiner/git/software/pybind11/tests/test_sequences_and_iterators.cpp:39:5: note: 'NonCopyableInt' has been explicitly marked deleted here
    NonCopyableInt(const NonCopyableInt &) = delete;
    ^
In file included from /Users/henryschreiner/git/software/pybind11/tests/test_sequences_and_iterators.cpp:11:
In file included from /Users/henryschreiner/git/software/pybind11/tests/pybind11_tests.h:3:
/Users/henryschreiner/git/software/pybind11/include/pybind11/pybind11.h:1991:16: error: call to deleted constructor of 'pybind11::detail::iterator_value_access<std::__wrap_iter<std::pair<NonCopyableInt, NonCopyableInt> *>, NonCopyableInt>::result_type' (aka 'NonCopyableInt')
        return (*it).second;
               ^~~~~~~~~~~~
/Users/henryschreiner/git/software/pybind11/include/pybind11/pybind11.h:2017:24: note: in instantiation of member function 'pybind11::detail::iterator_value_access<std::__wrap_iter<std::pair<NonCopyableInt, NonCopyableInt> *>, NonCopyableInt>::operator()' requested here
                return Access()(s.it);
                       ^
/Users/henryschreiner/git/software/pybind11/include/pybind11/pybind11.h:2068:20: note: in instantiation of function template specialization 'pybind11::detail::make_iterator_impl<pybind11::detail::iterator_value_access<std::__wrap_iter<std::pair<NonCopyableInt, NonCopyableInt> *>, NonCopyableInt>, pybind11::return_value_policy::reference_internal, std::__wrap_iter<std::pair<NonCopyableInt, NonCopyableInt> *>, std::__wrap_iter<std::pair<NonCopyableInt, NonCopyableInt> *>, NonCopyableInt>' requested here
    return detail::make_iterator_impl<
                   ^
/Users/henryschreiner/git/software/pybind11/tests/test_sequences_and_iterators.cpp:354:24: note: in instantiation of function template specialization 'pybind11::make_value_iterator<pybind11::return_value_policy::reference_internal, std::__wrap_iter<std::pair<NonCopyableInt, NonCopyableInt> *>, std::__wrap_iter<std::pair<NonCopyableInt, NonCopyableInt> *>, NonCopyableInt>' requested here
            return py::make_value_iterator(vec.begin(), vec.end());
                       ^
/Users/henryschreiner/git/software/pybind11/tests/test_sequences_and_iterators.cpp:39:5: note: 'NonCopyableInt' has been explicitly marked deleted here
    NonCopyableInt(const NonCopyableInt &) = delete;
    ^
2 errors generated.

henryiii · 2021-09-22T21:17:29Z

Fixed most of them by adding the double parentheses back in - should probably make a note that double parentheses are required there. Don't know why, would like to be able to say why - it was true across all compilers. Looks like at least ICC is unhappy now, but most are fine. Edit: ee0c5ee#r56899065 explains the parens.

Skylion007 · 2021-09-22T21:29:09Z

Ping @rainwoodman

henryiii · 2021-09-22T21:35:45Z

These are the three compilers that can't handle it:

X  🐍 3 • CentOS7 / PGI 20.9 • x64                            15m19s  https://github.com/pybind/pybind11/runs/3680311051
X  🐍 3 • ICC latest • x64                                    4m56s   https://github.com/pybind/pybind11/runs/3680313285
X  🐍 3.8 • CUDA 11 • Ubuntu 20.04                            5m23s   https://github.com/pybind/pybind11/runs/3680312770
X  🐍 3.9-dbg (deadsnakes) • Valgrind • x64                   14m39s  https://github.com/pybind/pybind11/runs/3680309032

PGI, NVCC, Intel ICC, which have had some historical problems with avoiding copies; and I expect my test is triggering valgrind. Also okay to revert #3271 then try again with this on top, if @bmerry is not available to fix this till next week and no one can fix these compilers. This is a pretty serous regression in master that was discovered <24 hours by a user after merging.

(Note that a "fix" could simply mean not compiling this "non-copyable" test on those compilers, but some attempt should be made to understand/fix first. Can also not run the test breaking valgrind at runtime, as it doesn't really do anything).

rwgk · 2021-09-22T21:52:21Z

Reverting sounds good to me.

…

On Wed, Sep 22, 2021 at 17:35 Henry Schreiner ***@***.***> wrote: These are the three compilers that can't handle it: X 🐍 3 • CentOS7 / PGI 20.9 • x64 15m19s https://github.com/pybind/pybind11/runs/3680311051 X 🐍 3 • ICC latest • x64 4m56s https://github.com/pybind/pybind11/runs/3680313285 X 🐍 3.8 • CUDA 11 • Ubuntu 20.04 5m23s https://github.com/pybind/pybind11/runs/3680312770 X 🐍 3.9-dbg (deadsnakes) • Valgrind • x64 14m39s https://github.com/pybind/pybind11/runs/3680309032 PGI, NVCC, Intel ICC, and I expect my test is triggering valgrind. Also okay to revert #3271 <#3271> then try again with this on top, if @bmerry <https://github.com/bmerry> is not available to fix this till next week and no one can fix these compilers. This is a pretty serous regression in master that was discovered <24 hours after merging by a user. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#3293 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFUZAEH62O2TMNLWWLMHH3UDJD3XANCNFSM5ESC7HXQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

include/pybind11/pybind11.h

henryiii · 2021-09-23T02:49:42Z

Going to revert, then move the rest of the changes into here.

henryiii · 2021-09-23T03:15:58Z

Ideally, tests should be pulled out and applied to master first.

bmerry · 2021-09-23T06:39:24Z

Hi @henryiii. Thanks for working on this - seems like you have more time for it this week than I do. From a quick look you're taking a similar approach to what I was thinking of. However, I'd suggest you try not using the ResultType template parameter to define the result_type nested typedef (I'll make a code suggestion to show what I mean). You're hitting the same problem I hit originally, where ICC (and I'm guessing the other failing compilers) gets the wrong type when decltype is used as the template parameter default (it loses the referenceness). That shouldn't matter if it's only used for SFINAE.

bmerry · 2021-09-23T06:41:42Z

include/pybind11/pybind11.h

+template <typename Iterator, typename ResultType = decltype((*std::declval<Iterator>()))>
+struct iterator_access {
+    using result_type = ResultType;


Suggested change

template <typename Iterator, typename ResultType = decltype((*std::declval<Iterator>()))>

struct iterator_access {

using result_type = ResultType;

template <typename Iterator, typename SFINAE = decltype((*std::declval<Iterator>()))>

struct iterator_access {

// Note: can't use SFINAE template parameter to define result_type due to a bug in ICC

using result_type = decltype((*std::declval<Iterator>()));

See if that works (will need to be applied similarly to iterator_key_access and iterator_value_access.

I believe this fixes all three "special" compilers, but breaks every version of MSVC.

D:\a\pybind11\pybind11\include\pybind11/pybind11.h(1991,1): error C2440: 'return': cannot convert from 'const _Ty2' to 'const int &&' [D:\a\pybind11\pybind11\tests\pybind11_tests.vcxproj] with [ _Ty2=int ] D:\a\pybind11\pybind11\include\pybind11/pybind11.h(1991,21): message : You cannot bind an lvalue to an rvalue reference [D:\a\pybind11\pybind11\tests\pybind11_tests.vcxproj] D:\a\pybind11\pybind11\include\pybind11/pybind11.h(1990): message : while compiling class template member function 'const int &&pybind11::detail::iterator_value_access<Iterator,const _Ty2 &&>::operator ()(Iterator &) const' [D:\a\pybind11\pybind11\tests\pybind11_tests.vcxproj] with [ Iterator=const std::pair<int,int> *, _Ty2=int ]

That is extremely weird. Is this just by adding the SFINAE template parameter (relative to what was in #3271), or is it some interaction with the additional testing you added?

I see you've tried to make a workaround with add_lvalue_reference. I suspect that is going to cause other problems if the iterator dereferencing doesn't produce a reference (such as an input iterator). The next thing I would try is changing std::declval<Iterator>() to std::declval<Iterator &>() (all over) since the former will have type Iterator && while the latter will have type Iterator &. I don't see why applying dereferencing to an rvalue reference should give another rvalue reference though.

I think it's the additional testing, though I could be wrong. I think after adding the tests I've not been able to get MSVC to work. SFINAE seems to be working, it's just failing inside the correct overload. This would be easier if I set this up with MSVC locally on a Windows box. With nox it might be easier to setup than in the past. add_lvalue_reference and adding a & should be very similar except for void, so I'm expecting similar results, but let's see.

If we have an MSVC solution and an everyone-else solution, that would be doable.

add_lvalue_reference and adding a & should be very similar except for void, so I'm expecting similar results, but let's see.

Not really - you were using add_lvalue_reference to modify the result type, whereas I'm proposing modifying the iterator type. But since I don't really understand what MSVC was doing I'm also going to wait and see. I'm trying to use compiler explorer to reproduce the issue (which was pretty handy for the ICC bug), but haven't managed it yet.

Ah, yes, I see that now. Looks like this is working a lot better! 95% complete with no failures yet.

* Add make_value_iterator This is the counterpart to make_key_iterator, and will allow implementing a `value` method in `bind_map` (although doing so is left for a subsequent PR). I made a few design changes to reduce copy-and-paste boilerplate. Previously detail::iterator_state had a boolean template parameter to indicate whether it was being used for make_iterator or make_key_iterator. I replaced the boolean with a class that determines how to dereference the iterator. This allows for a generic implementation of `__next__`. I also added the ValueType and Extra... parameters to the iterator_state template args, because I think it was a bug that they were missing: if make_iterator is called twice with different values of these, only the first set has effect (because the state class is only registered once). There is still a potential issue in that the *values* of the extra arguments are latched on the first call, but since most policies are empty classes this should be even less common. * Add some remove_cv_t to appease clang-tidy * Make iterator_access and friends take reference For some reason I'd accidentally made it take a const value, which caused some issues with third-party packages. * Another attempt to remove remove_cv_t from iterators Some of the return types were const (non-reference) types because of the pecularities of decltype: `decltype((*it).first)` is the *declared* type of the member of the pair, rather than the type of the expression. So if the reference type of the iterator is `pair<const int, int> &`, then the decltype is `const int`. Wrapping an extra set of parentheses to form `decltype(((*it).first))` would instead give `const int &`. This means that the existing make_key_iterator actually returns by value from `__next__`, rather than by reference. Since for mapping types, keys are always const, this probably hasn't been noticed, but it will affect make_value_iterator if the Python code tries to mutate the returned objects. I've changed things to use double parentheses so that make_iterator, make_key_iterator and make_value_iterator should now all return the reference type of the iterator. I'll still need to add a test for that; for now I'm just checking whether I can keep Clang-Tidy happy. * Add back some NOLINTNEXTLINE to appease Clang-Tidy This is favoured over using remove_cv_t because in some cases a const value return type is deliberate (particularly for Eigen). * Add a unit test for iterator referencing Ensure that make_iterator, make_key_iterator and make_value_iterator return references to the container elements, rather than copies. The test for make_key_iterator fails to compile on master, which gives me confidence that this branch has fixed it. * Make the iterator_access etc operator() const I'm actually a little surprised it compiled at all given that the operator() is called on a temporary, but I don't claim to fully understand all the different value types in C++11. * Attempt to work around compiler bugs https://godbolt.org/ shows an example where ICC gets the wrong result for a decltype used as the default for a template argument, and CI also showed problems with PGI. This is a shot in the dark to see if it fixes things. * Make a test constructor explicit (Clang-Tidy) * Fix unit test on GCC 4.8.5 It seems to require the arguments to the std::pair constructor to be implicitly convertible to the types in the pair, rather than just requiring is_constructible. * Remove DOXYGEN_SHOULD_SKIP_THIS guards Now that a complex decltype expression has been replaced by a simpler nested type, I'm hoping Doxygen will be able to build it without issues. * Add comment to explain iterator_state template params

Skylion007

Looks good. May want to add a few comments explaining the black magic with the decltypes and how the & and the parens are needed.

henryiii · 2021-09-23T16:09:09Z

Just to be sure, I'm going to verify that using ResultType still breaks those three compilers, now that this is fixed. I'll also add a comment as to why this is needed if it still is. I'll keep it in draft until then.

henryiii · 2021-09-23T16:10:43Z

@Skylion007 do we need a couple more NOLINTNEXTLINE(readability-const-return-type) too? Not getting any linting errors, though.

bmerry · 2021-09-23T16:19:06Z

I've managed to reproduce the MSVC bug in Compiler Explorer: https://godbolt.org/z/343qzxe6z.

bmerry · 2021-09-23T16:22:03Z

It looks like this is a known bug in MSVC: https://developercommunity2.visualstudio.com/t/decltypepointerToArray-produces-rva/291161?entry=problem&space=62

henryiii · 2021-09-23T16:25:58Z

It looks like this is a known bug in MSVC:

Doesn't that say fixed in 2019, specifically VS16.9, but it still seems to be present in VS16.11?

henryiii · 2021-09-23T17:59:36Z

@rwgk and @bmerry okay to merge?

tests/test_sequences_and_iterators.cpp

bmerry · 2021-09-23T18:21:34Z

It looks like this is a known bug in MSVC:

Doesn't that say fixed in 2019, specifically VS16.9, but it still seems to be present in VS16.11?

You're right. So presumably it's not exactly the same bug, but some variant of it.

include/pybind11/pybind11.h

henryiii · 2021-09-23T18:30:29Z

You're right. So presumably it's not exactly the same bug, but some variant of it.

Are you going to report it? Especially with a nice reproducer, should be easy to report?

rwgk

I was not able to follow the details, but with so much attention given to this work, by so many people, I'm sure this will be fine!
After it's merged I'll run the google testing again (tonight). I think it's safe to not block on waiting for the results.

bmerry · 2021-09-23T18:50:21Z

You're right. So presumably it's not exactly the same bug, but some variant of it.

Are you going to report it? Especially with a nice reproducer, should be easy to report?

Yep: https://developercommunity2.visualstudio.com/t/decltype-infers-rvalue-reference-when-de/1537933?entry=myfeedback&space=62

Do any of you have any experience reporting ICC bugs? I tried searching for a bug reporting system, and the most useful thing I found was a forum rant from a user complaining that the only way to report bugs was on the forum and that the bug reports were usually ignored, so I didn't look any further.

bmerry

Thanks again for fixing this up for me @henryiii. Assuming the CI passes I reckon it's good to go. I've got some work in progress on wiring this up to bind_map that I'll just need to rebase and write tests for next week.

henryiii · 2021-09-23T19:04:52Z

They aren't all that great at responding to them, as they are leaving ICC behind for the new clang-based Intel compiler. I think @ax3l has reported before?

henryiii · 2021-09-23T19:06:18Z

Thanks all!

rainwoodman · 2021-09-26T22:17:18Z

Some reflection after the merge. I am astonished by the amount of C++ quirks we ran into for a relatively straightforward code reuse.

Could it have been easier if we instead pursued three separate function bodies, one for each of make_iterator, make_key_iterator, and make_value_iterator?

henryiii · 2021-09-26T22:24:25Z

SFINAE would have still been broken (the reuse part actually worked seamlessly). Trying to support non-copyable on all compilers was where much of the effort came in. Now if there had simply been different names for the one that takes an iterator and a sentinel and the one that takes a object supporting iteration, it would have been really easy, as then SFINAE would not have been needed. But that was not the case.

Welcome to C++. :D

rainwoodman · 2021-09-27T06:49:09Z

Thanks for the explanation!

henryiii marked this pull request as draft September 22, 2021 19:51

Skylion007 requested a review from rwgk September 22, 2021 21:27

henryiii force-pushed the henryiii/fix/regress_iter branch from f1e6b20 to b7338ac Compare September 22, 2021 22:08

henryiii commented Sep 23, 2021

View reviewed changes

include/pybind11/pybind11.h Outdated Show resolved Hide resolved

henryiii mentioned this pull request Sep 23, 2021

revert: Add make_value_iterator #3295

Merged

henryiii force-pushed the henryiii/fix/regress_iter branch from 14e4f26 to 06a9632 Compare September 23, 2021 03:14

henryiii changed the title ~~fix: regression in #3271~~ feat: reapply fixed version of #3271 Sep 23, 2021

bmerry reviewed Sep 23, 2021

View reviewed changes

henryiii force-pushed the henryiii/fix/regress_iter branch from 06a9632 to e850621 Compare September 23, 2021 13:37

henryiii force-pushed the henryiii/fix/regress_iter branch 2 times, most recently from ada6b3e to 760b442 Compare September 23, 2021 15:43

henryiii marked this pull request as ready for review September 23, 2021 16:05

henryiii marked this pull request as draft September 23, 2021 16:07

Skylion007 approved these changes Sep 23, 2021

View reviewed changes

henryiii force-pushed the henryiii/fix/regress_iter branch from 760b442 to 755133b Compare September 23, 2021 16:14

henryiii force-pushed the henryiii/fix/regress_iter branch from 755133b to 93c0a38 Compare September 23, 2021 16:28

henryiii marked this pull request as ready for review September 23, 2021 16:29

henryiii force-pushed the henryiii/fix/regress_iter branch 2 times, most recently from eab5682 to bde20cd Compare September 23, 2021 17:22

bmerry suggested changes Sep 23, 2021

View reviewed changes

tests/test_sequences_and_iterators.cpp Outdated Show resolved Hide resolved

bmerry reviewed Sep 23, 2021

View reviewed changes

tests/test_sequences_and_iterators.cpp Outdated Show resolved Hide resolved

henryiii force-pushed the henryiii/fix/regress_iter branch from bde20cd to 42ada15 Compare September 23, 2021 18:22

bmerry suggested changes Sep 23, 2021

View reviewed changes

include/pybind11/pybind11.h Outdated Show resolved Hide resolved

fix: regression in pybind#3271

db6e0d3

henryiii force-pushed the henryiii/fix/regress_iter branch from 42ada15 to db6e0d3 Compare September 23, 2021 18:29

rwgk approved these changes Sep 23, 2021

View reviewed changes

bmerry approved these changes Sep 23, 2021

View reviewed changes

henryiii merged commit 21282e6 into pybind:master Sep 23, 2021

henryiii deleted the henryiii/fix/regress_iter branch September 23, 2021 19:06

github-actions bot added the needs changelog Possibly needs a changelog entry label Sep 23, 2021

henryiii mentioned this pull request Sep 24, 2021

fix: iteration of non-copyable #3297

Closed

henryiii removed the needs changelog Possibly needs a changelog entry label Sep 24, 2021

bmerry mentioned this pull request Sep 27, 2021

Add .keys and .values to bind_map #3310

Merged

wolfv mentioned this pull request Oct 20, 2021

Compile problem under conda environment scikit-geometry/scikit-geometry#81

Closed

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#3293

Closed

feat: reapply fixed version of #3271 #3293

feat: reapply fixed version of #3271 #3293

Uh oh!

Conversation

henryiii commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Suggested changelog entry:

Uh oh!

henryiii commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henryiii commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Skylion007 commented Sep 22, 2021

Uh oh!

henryiii commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwgk commented Sep 22, 2021 via email

Uh oh!

Uh oh!

henryiii commented Sep 23, 2021

Uh oh!

henryiii commented Sep 23, 2021

Uh oh!

bmerry commented Sep 23, 2021

Uh oh!

bmerry Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

henryiii Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

bmerry Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

henryiii Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

bmerry Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

henryiii Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Skylion007 left a comment

Choose a reason for hiding this comment

Uh oh!

henryiii commented Sep 23, 2021

Uh oh!

henryiii commented Sep 23, 2021

Uh oh!

bmerry commented Sep 23, 2021

Uh oh!

bmerry commented Sep 23, 2021

Uh oh!

henryiii commented Sep 23, 2021

Uh oh!

henryiii commented Sep 23, 2021

Uh oh!

Uh oh!

Uh oh!

bmerry commented Sep 23, 2021

Uh oh!

Uh oh!

henryiii commented Sep 23, 2021

Uh oh!

rwgk left a comment

Choose a reason for hiding this comment

Uh oh!

bmerry commented Sep 23, 2021

Uh oh!

bmerry left a comment

Choose a reason for hiding this comment

Uh oh!

henryiii commented Sep 23, 2021

Uh oh!

henryiii commented Sep 22, 2021 •

edited

Loading

henryiii commented Sep 22, 2021 •

edited

Loading

henryiii commented Sep 22, 2021 •

edited

Loading

henryiii commented Sep 22, 2021 •

edited

Loading

henryiii Sep 23, 2021 •

edited

Loading

henryiii commented Sep 26, 2021 •

edited

Loading