Implement most of the C++ special member function rules #843

bsilver8192 · 2022-02-27T05:47:18Z

This now tracks most of the information about which C++ special member
functions are implicit/explicit/deleted/etc for most of the common
cases. This information was needed in several places, which were each
using different approximations that failed in different ways, so unify
those to get it all working. Also add a bunch of tests around the
various cases to keep this working.

This assumes that any non-analyzed types (except some built-in ones
which are handled specially) have no implicit special member functions,
instead of the previous behavior which assumed they all existed if the
analyzed type had explicit declarations. This should generate
functional code for more situations, but it will skip some optional
things (such as moveit traits and make_unique) for additional types. If
you run into issues with those things disappearing after this change,
make sure all dependencies of the type (superclasses and member types)
have a generate!/generate_pod!.

Added TODOs for the following unhandled parts:

Explicitly defaulted default/copy/move constructors are treated as present #815 (this is a Clang warning anyways, TODOs show all
the places to change to fix it)
Member initializers are ignored when checking for default constructors #816 (this just means we ignore some implicit
constructors which do exist)

Also added TODOs related to the followig issues, which limit what can be
tested but aren't made better or worse by this change:

Base classes without member variables are ignored #832 (this one affects lots of areas)
Types with inaccessible destructors may still be freed by Rust #829 (this one's pretty prone to unexpected behavior)

Also fixed some existing bugs which are surfaced by generating more
trait implementations for types in the existing tests:

Use the correct C++ name for destructors of nested types
Handle trait methods for types that end up ignored

Tests pass
Appropriate changes to README are included in PR

engine/src/conversion/analysis/fun/implicit_constructors.rs

bsilver8192 · 2022-02-27T08:41:04Z

@philipcraig could you help me understand the error in "Windows (msvc)-test" (https://github.com/google/autocxx/runs/5349131420?check_suite_focus=true)? I see that compilation failed, but I don't see any messages indicating why. I don't have any experience with MSVC, maybe I'm just overlooking it?

This PR should avoid generating UniquePtr instantiations for types without accessible destructors, and the failing test is for that behavior. Given that you had to fix a related issue in #841 that Linux GCC compiled anyways, I wonder if differences in how MSVC handles that part of C++ are involved.

philipcraig · 2022-02-27T09:20:07Z

@philipcraig could you help me understand the error in "Windows (msvc)-test" (https://github.com/google/autocxx/runs/5349131420?check_suite_focus=true)? I see that compilation failed, but I don't see any messages indicating why. I don't have any experience with MSVC, maybe I'm just overlooking it?

This PR should avoid generating UniquePtr instantiations for types without accessible destructors, and the failing test is for that behavior. Given that you had to fix a related issue in #841 that Linux GCC compiled anyways, I wonder if differences in how MSVC handles that part of C++ are involved.

Happy to help. The error is there in the CI if you search the CI output for the failing test name (test_tricky_destructors).

C:\Users\RUNNER~1\AppData\Local\Temp\.tmpIr1ld2\input.h(13): error C3861: '__builtin_trap': identifier not found
cl : Command line warning D9002 : ignoring unknown option '-std=c++14'
cl : Command line warning D9002 : ignoring unknown option '-std=c++14'

which comes from this code in the PR (in the test): https://github.com/google/autocxx/pull/843/files#diff-7d1a694c0fef109924a3e6d7c3462f13a4727cfa3b2a2f86620734ca3e1800fcR9381

Note you will have to get the Github UI to expand integration_test.rs as by default it hides it as the diff is large.

__builtin_trap is a gcc/clang specific builtin, not standard C++, so fails on msvc. I'm not sure if you really wanted it in your test -- was it left in for debugging purposes?

bsilver8192 · 2022-02-27T10:16:10Z

Happy to help. The error is there in the CI if you search the CI output for the failing test name (test_tricky_destructors).
C:\Users\RUNNER~1\AppData\Local\Temp\.tmpIr1ld2\input.h(13): error C3861: '__builtin_trap': identifier not found
cl : Command line warning D9002 : ignoring unknown option '-std=c++14'
cl : Command line warning D9002 : ignoring unknown option '-std=c++14'

Thanks, I hadn't realized that between GitHub and cargo test half the information is down at the bottom and half of it is up when the test was run...

__builtin_trap is a gcc/clang specific builtin, not standard C++, so fails on msvc. I'm not sure if you really wanted it in your test -- was it left in for debugging purposes?

I want abort() there, I did it the GCC-specific way without thinking. Thanks!

Although now I'm stuck again with https://github.com/google/autocxx/runs/5349490901?check_suite_focus=true. All I see is a red error in here:

test C:\Users\RUNNER~1\AppData\Local\Temp\.tmp5zpBBl\input.rs ... error: 'rustfmt.exe' is not installed for the toolchain 'nightly-x86_64-pc-windows-msvc'
To install, run `rustup component add rustfmt`
gen0.cxx
error
cl : Command line warning D9002 : ignoring unknown option '-std=c++14'
cl : Command line warning D9002 : ignoring unknown option '-std=c++14'
test integration_test::test_various_emplacement ... FAILED
Execution of the test case was unsuccessful but there was no output.

philipcraig · 2022-02-27T12:59:35Z

I'm stuck again with https://github.com/google/autocxx/runs/5349490901?check_suite_focus=true. All I see is a red error in here:
gen0.cxx
error
test integration_test::test_various_emplacement ... FAILED
Execution of the test case was unsuccessful but there was no output.

I don't know what to do next here either -- it seems some test output from failure is lost, maybe? Reading the source, seems like the Rust code has assertions. I wonder if when they fail we see this?

Could it be related to the s2 example failing to compile on all the triples, or you think that's separate?

In other tricky news, I've built your branch on a Windows host I have access to-- the test passes there, both when run individually or in the overall batch. That of course doesn't mean that everything is necessarily correct -- if there's undefined behaviour somewhere, it doesn't have to crash everywhere. It doesn't help in understanding what's going on though.

adetaylor · 2022-02-27T18:38:22Z

Hi @bsilver8192, thanks for this. I'm really excited that you've done such a systematic job here.

I'm afraid it's going to conflict slightly with a PR I've been working on for about a month - #766. I've split that into multiple PRs to get the conflicting bits in place so you can build on them. Specifically the FnPhase analysis phase is now split into two, so your FnStructAnalysis tweak will need to be adjusted a bit.

I also see unexplained problems in the Windows tests. Until we have completed #819 and have a full understanding of how to diagnose and resolve Windows-specific failures, I'm OK with applying attributes to disable extra tests on Windows (after a reasonable amount of investigation).

I also want to #844 to determine whether we have (some types of) UB.

bsilver8192 · 2022-02-27T22:46:52Z

I don't know what to do next here either -- it seems some test output from failure is lost, maybe? Reading the source, seems like the Rust code has assertions. I wonder if when they fail we see this?

On Linux, I always see a message when Rust assertions trigger, and a note about setting RUST_BACKTRACE to see the backtrace. I don't see either here.

Could it be related to the s2 example failing to compile on all the triples, or you think that's separate?

That's definitely separate. See the comment chain above for details. It's not generating Rust declarations that the test is testing the existence of, which means Rust compilation fails, which produces much more visible errors.

In other tricky news, I've built your branch on a Windows host I have access to-- the test passes there, both when run individually or in the overall batch. That of course doesn't mean that everything is necessarily correct -- if there's undefined behaviour somewhere, it doesn't have to crash everywhere. It doesn't help in understanding what's going on though.

I looked through that test and I don't see any undefined behavior. I verified on linux-gnu that the destructor is being run the correct number of times (which is more than before this PR; it used to just leak the A, including its std::string, from back_on_stack).

It passed in previous commits of this PR, and I've only made cosmetic changes since then. Any chance this is like #819 (comment), which talks about another test being flaky? The output looks the same, and both tests are moving things with moveit.

This now tracks most of the information about which C++ special member functions are implicit/explicit/deleted/etc for most of the common cases. This information was needed in several places, which were each using different approximations that failed in different ways, so unify those to get it all working. Also add a bunch of tests around the various cases to keep this working. This assumes that any non-analyzed types (except some built-in ones which are handled specially) have no implicit special member functions, instead of the previous behavior which assumed they all existed if the analyzed type had explicit declarations. This should generate functional code for more situations, but it will skip some optional things (such as moveit traits and make_unique) for additional types. If you run into issues with those things disappearing after this change, make sure all dependencies of the type (superclasses and member types) have a `generate!`/`generate_pod!`. Added TODOs for the following unhandled parts: * google#815 (this is a Clang warning anyways, TODOs show all the places to change to fix it) * google#816 (this just means we ignore some implicit constructors which do exist) Also added TODOs related to the followig issues, which limit what can be tested but aren't made better or worse by this change: * google#832 (this one affects lots of areas) * google#829 (this one's pretty prone to unexpected behavior) Also fixed some existing bugs which are surfaced by generating more trait implementations for types in the existing tests: * Use the correct C++ name for destructors of nested types * Handle trait methods for types that end up ignored

bsilver8192 · 2022-02-28T04:33:09Z

In other tricky news, I've built your branch on a Windows host I have access to-- the test passes there, both when run individually or in the overall batch. That of course doesn't mean that everything is necessarily correct -- if there's undefined behaviour somewhere, it doesn't have to crash everywhere. It doesn't help in understanding what's going on though.

I looked through that test and I don't see any undefined behavior. I verified on linux-gnu that the destructor is being run the correct number of times (which is more than before this PR; it used to just leak the A, including its std::string, from back_on_stack).

It passed in previous commits of this PR, and I've only made cosmetic changes since then. Any chance this is like #819 (comment), which talks about another test being flaky? The output looks the same, and both tests are moving things with moveit.

Well, current main is calling a C++ destructor on a pointer which never had the constructor called in another related test, which actually crashes for me (on linux-gnu). I'm going to fix that and send out a separate (hopefully smaller) PR so other people stop running into it, and hopefully that fixes all of them. asan should catch this reliably too.

bsilver8192 · 2022-02-28T05:25:54Z

Well, current main is calling a C++ destructor on a pointer which never had the constructor called in another related test, which actually crashes for me (on linux-gnu). I'm going to fix that and send out a separate (hopefully smaller) PR so other people stop running into it, and hopefully that fixes all of them. asan should catch this reliably too.

Looks like it was a recent change (#766 (comment)), so doesn't explain flakiness. I pushed my rebased version here if anybody wants to review.

This is actually pretty easy, and it eliminates the main source of new incompatibilities I can think of. Also remove the note about typedefs, they actually work fine.

adetaylor

Thanks for this! I've reviewed everything except the actual logic for creating constructors within implicit_constructors.rs. I've suggested you merge that file with implicit_constructor_rules.rs and then I'll take a look.

I've made some other requests but this is looking pretty great.

integration-tests/tests/integration_test.rs

engine/src/known_types.rs

engine/src/conversion/codegen_cpp/mod.rs

engine/src/conversion/analysis/remove_ignored.rs

engine/src/conversion/analysis/fun/mod.rs

engine/src/conversion/analysis/fun/implicit_constructors.rs

engine/src/conversion/codegen_rs/impl_item_creator.rs

Split out from google#843.

adetaylor

A few tiny nits/questions but this is basically ready to land.

engine/src/conversion/error_reporter.rs

engine/src/conversion/analysis/fun/mod.rs

engine/src/conversion/analysis/fun/implicit_constructors.rs

adetaylor · 2022-03-03T06:33:07Z

OK, assuming this all passes the CI, I'll go ahead and merge this tomorrow. Thanks again for all this excellent work!

bsilver8192 · 2022-03-03T06:34:32Z

Awesome! Thanks for maintaining this, I'm impressed at how much C++ it handles already, and how much more it does every day.

bsilver8192 · 2022-03-03T09:06:12Z

With my full use case, I'm getting C++ wrappers for constructors etc for _bindgen_ty_54-style types which don't actually exist, so the generated C++ doesn't compile. Looks like they're getting past the GC stage because many of the generated constructors in a given namespace share the QualifiedName(Namespace([]), "new_synthetic_move_ctor") name, so they all get lumped together.

I suspect it's something that can happen before this PR, but this PR makes it happen in many more situations. Although I'm not actually sure what situations create these _bindgen_ty_ types, so maybe they can never have explicit special members that would trigger code generation before this PR. That also means I don't know how common of an occurrence this problem is, so I don't know whether it makes sense to hold up this PR for it.

I can keep digging into this over the next few days if there isn't an easy answer.

adetaylor · 2022-03-04T23:17:57Z

engine/src/conversion/analysis/fun/mod.rs

+            if let Some(Api::Struct {
+                analysis: PodAndConstructorAnalysis { constructors, .. },
+                ..
+            }) = apis.iter_mut().find(|api| api.name() == self_ty)


I'm concerned this may O(n^2) over the number of APIs. I think I've avoided that elsewhere so far...

Looks like you fixed this one in #889, LGTM.

Never mind, you just cut the square in half, but the triangle is still O(n^2). This one is pretty easy to move into find_constructors_present instead, done.

adetaylor · 2022-03-04T23:31:57Z

engine/src/conversion/analysis/fun/mod.rs

+#[derive(Debug, Default, Copy, Clone)]
+pub(crate) struct PublicConstructors {
+    pub(crate) default_constructor: bool,
+    pub(crate) copy_constructor: bool,


As far as I can see, a couple of these fields are never read.

When I merge this with #879 this gives me compile-time warnings. I'm not sure why it doesn't here.

I think it's just this one, which ended up not being referenced by other stages. Done

adetaylor · 2022-03-04T23:59:45Z

engine/src/conversion/analysis/fun/mod.rs

+                let initial_name = name.clone();
+                // Now go find the Api::Struct for our type, and verify it has an accessible
+                // destructor.
+                for api in apis.iter() {


This also seems to be O(n^2)

adetaylor · 2022-03-05T00:13:30Z

OK, state of play:

Thanks for reporting the issues with non-unique API names. I have fixed them in Ensure every API has a unique name #879.
That's a substantial change, and of course courses merge conflicts. I have taken the liberty of resolving them with your changes here, and put the result in Implicit constructors, with unique API names #889. Feel free to just merge that branch into this PR if you like. (There are a lot of merge commits - it was a bit painstaking).
In the process I've spotted some more concerns in your PR - mentioned above.
Also, the merged version fails some tests. This is an interaction of your change and Workaround problem with nested type implicit constructors #886 which recently landed in main (in a related area, but discovered independently while trying to write doctests). A full fix to Implicit constructors for nested types were broken and are now disabled #884 would solve this, but we're in a chicken and egg situation: I don't want to attempt to solve that until your PR here has landed, because otherwise we'd get horrible merge conflicts. I fear the best option is for you to attempt to solve Implicit constructors for nested types were broken and are now disabled #884 on top of your branch... WDYT?

Using the implicit ones (like this PR does for non-nested types) requires some additional refactoring, so let's leave that for now (google#884).

bsilver8192 · 2022-03-05T03:21:14Z

Thanks for reporting the issues with non-unique API names. I have fixed them in Ensure every API has a unique name #879.

That's a substantial change, and of course courses merge conflicts. I have taken the liberty of resolving them with your changes here, and put the result in Implicit constructors, with unique API names #889. Feel free to just merge that branch into this PR if you like. (There are a lot of merge commits - it was a bit painstaking).

Thanks, that's an annoying merge.

Also, the merged version fails some tests. This is an interaction of your change and Workaround problem with nested type implicit constructors #886 which recently landed in main (in a related area, but discovered independently while trying to write doctests). A full fix to Implicit constructors for nested types were broken and are now disabled #884 would solve this, but we're in a chicken and egg situation: I don't want to attempt to solve that until your PR here has landed, because otherwise we'd get horrible merge conflicts. I fear the best option is for you to attempt to solve Implicit constructors for nested types were broken and are now disabled #884 on top of your branch... WDYT?

I got partway through fixing #884, and then realized that it's not actually necessary. I might have it finished later tonight, definitely sometime this weekend. Treating nested types the same way as types with unknown bases/members lets explicit special member functions work like before, so all the existing tests pass. WDYT about leaving that for a separate PR?

adetaylor · 2022-03-05T03:24:26Z

Leaving it till later sounds fine to me.

adetaylor · 2022-03-05T05:16:00Z

engine/src/conversion/apivec.rs

@@ -89,6 +89,10 @@ impl<P: AnalysisPhase> ApiVec<P> {
        self.apis.iter()
    }

+    pub(crate) fn iter_mut(&mut self) -> impl Iterator<Item = &mut Api<P>> {
+        self.apis.iter_mut()


Sorry, we can't do this. In fact I worked really hard to remove this in bcfeb6f! The problem is that it might invalidate the invariants which ApiVec promises to maintain.

I struggled for a while with a custom iterator which then re-checked the invariants on its Drop, but borrow checker fights caused me to give up, and rewrite calling code so it didn't need iter_mut. You're welcome to try that again.

WDYT about fn mutate_fun_analysis<F: Fn(&mut P::FunAnalysis)>(&mut self, f: F) (and another one for structs)? Kind of ugly, but those don't allow changing the names, so the invariants can't be invalidated.

Or, a bit less ugly but with a runtime assertion, fn filter_inplace<F: Fn(&mut Api)>(&mut self, f: F) which assert!s that the name doesn't change after each function call.

Either would be fine with me, yes. I think on the whole I prefer the former, but I don't mind!

adetaylor · 2022-03-05T15:39:25Z

OK, I'm going to go ahead and merge this, but I would like one or other of us to try making a follow-up change to remove a little of the added complexity.

I'm not keen to add an extra field to MethodKind unless it's strictly necessary. I think in this case we can avoid it.

This is the sort of pattern that autocxx does in other places:
d213e4e
The power of hash tables to avoid O(n^2)...

That commit isn't quite complete because (a) it doesn't remove the newly-added field, (b) I haven't checked that it correctly respects all implicit + explicit destructors, but hopefully we can make this work.

Unfortunately I won't have any time at all to work on autocxx this weekend, hence going ahead and merging this to unblock you. Feel free to raise a PR completing this approach if you like, or I'm happy to take a look later. It might turn out not to be possible anyway.

Apart from that, this PR remains terrific, thanks a lot!

This should be the state after google#843 and google#891. I think documenting this will be helpful for people running this on a new codebase (or part of a codebase).

bsilver8192 · 2022-03-05T22:01:18Z

Bisecting by hand, it's a method that takes cpu_set_t by value which causes the problem. I'll dig more into what about that type is unique tomorrow.

A followup on this: I no longer see the bad destructors, not even pre-GC. I think the apivec-driven fixes addressed the root cause. Yay!

bsilver8192 commented Feb 27, 2022

View reviewed changes

engine/src/conversion/analysis/fun/implicit_constructors.rs Outdated Show resolved Hide resolved

bsilver8192 commented Feb 27, 2022

View reviewed changes

engine/src/conversion/analysis/fun/implicit_constructors.rs Outdated Show resolved Hide resolved

bsilver8192 force-pushed the special-member-functions branch from 29a0ed6 to 8943de2 Compare February 28, 2022 05:24

bsilver8192 added 2 commits February 28, 2022 00:17

Handle pointer and reference members

d812031

This is actually pretty easy, and it eliminates the main source of new incompatibilities I can think of. Also remove the note about typedefs, they actually work fine.

Merge remote-tracking branch 'upstream/main' into HEAD

baff8bf

adetaylor requested changes Mar 1, 2022

View reviewed changes

bsilver8192 added a commit to bsilver8192/autocxx that referenced this pull request Mar 2, 2022

Fix calling destructors of nested types

c99af9b

Split out from google#843.

This was referenced Mar 2, 2022

Fix calling destructors of nested types #864

Closed

Figure out how to differentiate between pointer and reference member variables #865

Closed

Code review feedback

e8abe78

adetaylor mentioned this pull request Mar 2, 2022

Nested destructor prob #867

Merged

Merge remote-tracking branch 'upstream/main' into HEAD

9beeae3

bsilver8192 mentioned this pull request Mar 3, 2022

Fix pin prob #855

Merged

adetaylor mentioned this pull request Mar 3, 2022

Try passing uncopyable types by value #870

Closed

Merge remote-tracking branch 'upstream/main' into HEAD

b8fce18

adetaylor reviewed Mar 3, 2022

View reviewed changes

engine/src/conversion/error_reporter.rs Show resolved Hide resolved

engine/src/conversion/analysis/fun/mod.rs Outdated Show resolved Hide resolved

engine/src/conversion/analysis/fun/implicit_constructors.rs Show resolved Hide resolved

Rename function

3e0ff15

adetaylor mentioned this pull request Mar 3, 2022

Synthesized constructors do not have unique names #877

Closed

adetaylor added 6 commits March 4, 2022 15:00

Merge commit 'bcfeb6f' into implicits-on-uniques2

ca83a2f

Merge commit 'f07fd72' into implicits-on-uniques2

25a8e62

Merge commit '97e7c80' into implicits-on-uniques2

dd50b52

Merge commit 'f3fbaeb' into implicits-on-uniques2

42e673c

Merge commit 'c693b93' into implicits-on-uniques2

2e9b13c

Merge commit '9171223' into implicits-on-uniques2

449715c

adetaylor reviewed Mar 4, 2022

View reviewed changes

adetaylor added 3 commits March 4, 2022 15:26

Merge commit 'dd3ee55' into implicits-on-uniques2

75b9455

Merge commit '5c90b18' into implicits-on-uniques2

7cb1471

Merge commit '9ac5b09' into implicits-on-uniques2

bc9c3b1

adetaylor reviewed Mar 4, 2022

View reviewed changes

adetaylor mentioned this pull request Mar 4, 2022

Implicit constructors, with unique API names #889

Merged

adetaylor reviewed Mar 4, 2022

View reviewed changes

bsilver8192 added 2 commits March 4, 2022 18:33

Some code review feedback

4925a6d

Still use explicit special members for nested types

e272be9

Using the implicit ones (like this PR does for non-nested types) requires some additional refactoring, so let's leave that for now (google#884).

bsilver8192 added 3 commits March 4, 2022 19:52

More code review feedback

d2ce463

Fix test

b295aca

Ignore orphan methods

523e61e

adetaylor reviewed Mar 5, 2022

View reviewed changes

No ApiVec::iter_mut

030ea55

adetaylor merged commit 7bd351c into google:main Mar 5, 2022

adetaylor mentioned this pull request Mar 5, 2022

Hopefully can remove the newly added type_constructors field #890

Closed

bsilver8192 deleted the special-member-functions branch March 5, 2022 20:46

bsilver8192 mentioned this pull request Mar 5, 2022

Document the current state of special member functions #892

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement most of the C++ special member function rules #843

Implement most of the C++ special member function rules #843

bsilver8192 commented Feb 27, 2022 •

edited

Loading

bsilver8192 commented Feb 27, 2022

philipcraig commented Feb 27, 2022 •

edited

Loading

bsilver8192 commented Feb 27, 2022

philipcraig commented Feb 27, 2022 •

edited

Loading

adetaylor commented Feb 27, 2022

bsilver8192 commented Feb 27, 2022

bsilver8192 commented Feb 28, 2022

bsilver8192 commented Feb 28, 2022

adetaylor left a comment

adetaylor left a comment

adetaylor commented Mar 3, 2022

bsilver8192 commented Mar 3, 2022

bsilver8192 commented Mar 3, 2022

adetaylor Mar 4, 2022

bsilver8192 Mar 5, 2022

bsilver8192 Mar 5, 2022

adetaylor Mar 4, 2022

bsilver8192 Mar 5, 2022

adetaylor Mar 4, 2022

bsilver8192 Mar 5, 2022

adetaylor commented Mar 5, 2022 •

edited

Loading

bsilver8192 commented Mar 5, 2022

adetaylor commented Mar 5, 2022

adetaylor Mar 5, 2022

bsilver8192 Mar 5, 2022

bsilver8192 Mar 5, 2022

adetaylor Mar 5, 2022

bsilver8192 Mar 5, 2022

adetaylor commented Mar 5, 2022

bsilver8192 commented Mar 5, 2022

Implement most of the C++ special member function rules #843

Implement most of the C++ special member function rules #843

Conversation

bsilver8192 commented Feb 27, 2022 • edited Loading

bsilver8192 commented Feb 27, 2022

philipcraig commented Feb 27, 2022 • edited Loading

bsilver8192 commented Feb 27, 2022

philipcraig commented Feb 27, 2022 • edited Loading

adetaylor commented Feb 27, 2022

bsilver8192 commented Feb 27, 2022

bsilver8192 commented Feb 28, 2022

bsilver8192 commented Feb 28, 2022

adetaylor left a comment

Choose a reason for hiding this comment

adetaylor left a comment

Choose a reason for hiding this comment

adetaylor commented Mar 3, 2022

bsilver8192 commented Mar 3, 2022

bsilver8192 commented Mar 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adetaylor commented Mar 5, 2022 • edited Loading

bsilver8192 commented Mar 5, 2022

adetaylor commented Mar 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adetaylor commented Mar 5, 2022

bsilver8192 commented Mar 5, 2022

bsilver8192 commented Feb 27, 2022 •

edited

Loading

philipcraig commented Feb 27, 2022 •

edited

Loading

philipcraig commented Feb 27, 2022 •

edited

Loading

adetaylor commented Mar 5, 2022 •

edited

Loading