std.container.Array: Payload "default constructor" simplifies Array. #8162

DoctorNoobingstoneIPresume · 2021-07-09T16:28:52Z

The purpose is reduce code duplication.
Thank you for considering this.

dlang-bot · 2021-07-09T16:28:53Z

Thanks for your pull request and interest in making D better, @DoctorNoobingstoneIPresume! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8162"

dkorpel

Love seeing a red diff! But maybe someone more familiar with Array than me should check whether the duplicated code didn't have subtle differences that were important e.g. for performance.

dkorpel · 2021-07-11T12:48:02Z

std/container/array.d

+pure @system unittest
+{
+    immutable(size_t) n = 100;
+    float[] a0; a0.length = n; a0[] = 1.41421f;


One statement per line according to D-style

I felt that all statements really were part of the declaration-and-initialization of a0.

Anyway, I have changed to one statement per line. I am using braces as an IMO idiomatic hint (for the human readers and for IDE's which support collapsing-and-expanding brace-delimited scopes) that those statements are only there to initialize the thing defined immediately above the scope (a0) and do not affect the rest of the code => also make it easy to factor out the code in a separate function.

I hope that is okay.

dkorpel · 2021-07-11T12:48:54Z

std/container/array.d

+    auto a1 = Array!float(a0);
+    assert(a1.length == n);
+    assert(a1.capacity == n); // Checks that a1's constructor has called reserve.
+    import std.algorithm.comparison;


Suggested change

import std.algorithm.comparison;

import std.algorithm.comparison : equal;

D-scanner wants selective imports in local scopes.

Okay, this is easy to change and I have changed it.

I am just wondering whether this is somehow redundant: We see equal twice, once in the import declaration and once in the actual call to equal.

I am adding that this is a small scope. But even if it were a larger scope, we could use braces in order to reduce the effect of the import declaration to a smaller scope => IMO annihilate bad effect of un-selective import while also removing redundancy and improving convenience.

What do you think, please ? (-:

dkorpel · 2021-07-11T12:55:41Z

std/container/array.d

-                GC.addRange(p, T.sizeof * values.length);
-        }
-        _data = Data(p[0 .. values.length]);
+        _data = Data(DefaultConstruct.value);


Suggested change

_data = Data(DefaultConstruct.value);

_data = Data.init;

No need to add a new public constructor that does nothing.

I would love this to work. We would have an idiom for a struct having some kind of a default constructor even in the presence of other constructors.

Unfortunately, this results in:

core.exception.AssertError@C:\Program Files (x86)\Dev\DigitalMars\dmd.2.096.1.windows\windows\bin\..\..\src\phobos\std\typecons.d(6631): Attempted to access an uninitialized payload. ---------------- 0x00421CE5 0x00408BA0 0x00402323 0x00421ABD 0x0042A8BB 0x00425870 0x00423677 0x00421C43 0x7D4E7CC2 in FormatMessageW 1/1 modules FAILED unittests C:\Program Files (x86)\Dev\DigitalMars\dmd.2.096.1.windows\windows\bin\..\..\src\phobos\std\typecons.d(6631): [unittest] Attempted to access an uninitialized payload.

dkorpel · 2021-07-11T12:56:27Z

std/container/array.d

@@ -24,6 +24,17 @@ import std.traits;

 public import std.container.util;

+pure @system unittest
+{
+    immutable(size_t) n = 100;


Suggested change

immutable(size_t) n = 100;

enum n = 100;

Or just use 100 directly since it's only used 3 times in 5 lines.

I have changed to enum size_t n = 100;. I feel that reading size_t (as opposed to simply reading uint or, even worse, int) is helpful for me, as an IMO idiomatic hint that n holds a "number of elements" (or the index of an element within a range/container). I hope that is okay.

dkorpel · 2021-07-11T13:00:30Z

std/container/array.d

-        {
-            _data.reserve(elements);
-        }
+            _data = Data(DefaultConstruct.value);


Suggested change

_data = Data(DefaultConstruct.value);

_data = Data.init;

dkorpel · 2021-07-11T13:06:23Z

std/container/array.d

+        _data = Data(DefaultConstruct.value);
+        _data.reserve(values.length);
+        foreach (ref value; values)
+            _data.insertBack(value);


insertBack has some overhead checking for capacity each time. Since you reserved the right length, you can just emplace each element directly like the original code does.

Thank you. I have updated accordingly.

DoctorNoobingstoneIPresume · 2021-07-13T02:30:31Z

Love seeing a red diff!

Thank you for constructive, encouraging and educative review.

This PR is a continuation of #8143. Both PRs have been motivated by seeing repeated sections of code, similar to the implementation of Payload.reserve.

But maybe someone more familiar with Array than me should check whether the duplicated code didn't have subtle differences that were important e.g. for performance.

Such checks (and all constructive reviews) are warmly welcome !

For now, I have tried to fix the performance problem spotted in your review (insertBack redundantly checking length vs capacity).

RazvanN7 · 2021-07-13T09:44:25Z

The buildkite failure might be fixed by a rebase.

dkorpel · 2021-07-13T10:03:51Z

std/container/array.d

+    assert(a1.length == n);
+    assert(a1.capacity == n); // Checks that a1's constructor has called reserve.
+    import std.algorithm.comparison : equal;
+    assert(equal(a1[], a0[]));


Try to make unittests focussed and self-contained if you can. If it's only testing that "a1's constructor has called reserve" then the complex initialization of the float array is not needed:

immutable int[4] a0 = [10, 20, 30, 40]; auto a1 = Array!int(a0[]); assert(a1.length == a0.length); assert(a1.capacity == a0.length); // Checks that a1's constructor has called reserve. assert(a1[] == a0[]);

Try to make unittests focussed and self-contained if you can. If it's only testing that "a1's constructor has called reserve" then the complex initialization of the float array is not needed:

immutable int[4] a0 = [10, 20, 30, 40]; auto a1 = Array!int(a0[]); assert(a1.length == a0.length); assert(a1.capacity == a0.length); // Checks that a1's constructor has called reserve. assert(a1[] == a0[]);

Unfortunately, for some lengths, the test passes even if reserve is not called (at least with the insertBack version, which was a correct version, even though it might have been less efficient than the emplace version).

I have now added even more comments in the source code for the unittest, so that others do not fall in the same traps I have.

While the line count has increased, IMO it is quite readable by humans. And (I dare say) useful.

DoctorNoobingstoneIPresume · 2021-07-13T15:50:02Z

The buildkite failure might be fixed by a rebase.

Thank you. Let us see !

DoctorNoobingstoneIPresume · 2021-07-13T16:16:36Z

I am very curious as to why Data.init cannot be used (#8162 (comment)),

i.e. what is it about RefCounted ! (...) which makes a constructor with an empty body differ in result from .init.

Currently, with my low skills and high laziness, it is going to take some possibly-infinite time to investigate.

DoctorNoobingstoneIPresume · 2021-07-13T17:24:07Z

Love seeing a red diff!

Thank you for constructive, encouraging and educative review.

This PR is a continuation of #8143. Both PRs have been motivated by seeing repeated sections of code, similar to the implementation of Payload.reserve.

But maybe someone more familiar with Array than me should check whether the duplicated code didn't have subtle differences that were important e.g. for performance.

Such checks (and all constructive reviews) are warmly welcome !

For now, I have tried to fix the performance problem spotted in your review (insertBack redundantly checking length vs capacity).

Another performance regression (in my initial PR) has been found (and, I hope, fixed): reserve(0) used to ensureInitialized the RefCountedStore object (and this used to negatively affect performance for the case of an uninitialized RefCountedStore object) !

BTW, in D, is the word "object" correct even for an instance of a struct type (not just for an instance of a class type), please ?

pbackus · 2021-08-19T21:25:21Z

std/container/array.d

+                // We increment the length after each iteration (as opposed to adjusting it just once, after the loop)
+                // in order to improve error-safety (in case one of the calls to emplace throws).


So if one of the calls to emplace throws, the Array will be partially populated, with some unspecified length <= values.length? This does not seem like desirable behavior, and as far as I can tell is not how it worked before. Please preserve the original behavior here.

@pbackus: First, I apologize for long delay. Second:

For this case -- when one of the calls to emplace throws -- after some (but not all) calls to emplace have succeeded:

the pre-existing behaviour is that the length remains unchanged -- just as if no elements have been added -- but IMO this is incorrect because some elements have been successfully added;

the proposed behaviour is that length is increased with the (some) number of elements which have been successfully added (indeed, a number less than values.length);

but length should not be increased with values.length, because that would signify that all requested elements have been successfully added.

Am I mis-understanding anything, please ?

Your understanding is correct. However, I disagree with your opinion that the pre-existing behavior is wrong.

If the user requests an array containing a specific list of values, we must either give them what they requested, or report failure via an exception. Giving them part of what they requested is not acceptable. Users must be able to rely on the standard library to do what they ask it to do.

It is also highly inappropriate to introduce a behavioral change like this in a PR whose stated purpose is to "reduce code duplication."

Your understanding is correct. However, I disagree with your opinion that the pre-existing behavior is wrong.

If the user requests an array containing a specific list of values, we must either give them what they requested, or report failure via an exception. Giving them part of what they requested is not acceptable. Users must be able to rely on the standard library to do what they ask it to do.

I agree that this constructor should either successfully construct an array containing all requested elements or propagate the Exception.

I believe that this is done both by pre-existing code and by the proposed changeset.

However !!

I believe that the pre-existing code incorrectly tracks the number of (successfully) constructed elements during the loop in the constructor. The tracked number of elements is zero during the entire loop and is only updated after the loop -- that is: if the entire loop has been successfully completed.

But if the loop is not completed:

yes, the Exception is propagated (the user is not misled into thinking that his/her request for constructing the Array has successfully completed when in fact fewer elements than requested have been emplaced in it) -- this is good (and is kept in the proposed changeset);

no, the already-constructed elements are not destroyed (because they have not been tracked) -- this is bad (and is corrected by the proposed changeset).

I have just added a unittest. With the pre-existing code, the unittest fails.

(The pre-existing code is temporarily enabled with static if (1) at line 553 in the current patchset, i.e. in DoctorNoobingstoneIPresume@cd46b4e. This is just temporary so we can easily test together: if we change that line to static if (0), the new code is enabled and the unittest passes.)

It is also highly inappropriate to introduce a behavioral change like this in a PR whose stated purpose is to "reduce code duplication."

I agree that the commit message (and possibly the code) can be improved. I have just tried to modify the commit message now. I hope it is better.

(The pre-existing code is temporarily enabled with static if (1) at line 553 in the current patchset, i.e. in DoctorNoobingstoneIPresume@cd46b4e. This is just temporary so we can easily test together: if we change that line to static if (0), the new code is enabled and the unittest passes.)

I have just changed that line to static if (0) in order to enable the new code: DoctorNoobingstoneIPresume@26ca49a.

BTW: In Github, is there a way to see all the commits that a proposed changeset has referred to during its evolution, please ?

An issue has been added as https://issues.dlang.org/show_bug.cgi?id=22337.

Could you please let me know whether the commit message of the PR should be modified to specify the issue number ?

Regarding making a separate PR: In this case, I believe this is just extra work for little benefit. With the current PR, we get both a fixed bug and simpler-and-more-clear code (if we do not count the unittest's and the comments). The simpler code (with fewer lines of code) helps us reason about the bug and the fix.

The commit message for a bug fix should be formatted according to these instructions: https://wiki.dlang.org/Contributing_to_Phobos#Issues

Using separate PRs for bug fixes and refactors makes the changes easier to review and makes the git history of the code more granular (which makes it easier to git bisect, for example). In any case, it is D policy not to mix refactors and bug fixes; see here: https://github.com/dlang/dmd/blob/master/CONTRIBUTING.md#refactoring

Is this not already very easy to review ?

How easy a PR is to review is a subjective matter. However, we do have objective guidelines and those are that refactorings and bug fixes should go to separate PRs. I suggest that you use this PR to do the refactoring and make a subsequent PR to fix the bug.

This matter has been extensively discussed and contributors occasionally have arguments against this policy, however, experience has shown that it is best to separate refactorings from bug fixes.

The "objective" guidelines referred above (https://github.com/dlang/dmd/blob/master/CONTRIBUTING.md#refactoring) state that:

"[...] Pull requests that do non-trivial refactorings should not include other changes, such as new feature or bug fix. [...]"

IMO, this "refactoring" is trivial.

RazvanN7 · 2021-09-13T10:16:29Z

@DoctorNoobingstoneIPresume Any updates on this?

DoctorNoobingstoneIPresume · 2021-09-24T19:09:27Z

@DoctorNoobingstoneIPresume Any updates on this?

@RazvanN7 @pbackus I apologize for the delay. I have only today resumed investigations. I hope to answer again soon.

pbackus · 2021-09-24T21:45:33Z

i.e. what is it about RefCounted ! (...) which makes a constructor with an empty body differ in result from .init.

In my experience the most likely cause of this is a nested struct.

Nested structs have a hidden context pointer that must be initialized at runtime. When you construct an instance using constructor syntax (e.g., MyStruct()), this initialization is performed. However, when you use .init, this initialization is skipped, and the context pointer is set to null instead. Here's an example:

unittest
{
    int x = 1;
    
    struct Nested
    {
        int y = 2;
        int fun() { return x + y; } // access context
    }

    Nested a = Nested(); // use constructor syntax
    assert(a.fun() == 3); // ok
    
    Nested b = Nested.init; // use .init
    assert(b.fun() == 3); // Segmentation fault
}

The reason for this is that the .init value of a type is always evaluated at compile time, not runtime.

This is another attempt to re-use the code in `Payload.reserve` -- similar to dlang#8143 (commit 0b7bf8d): "std.container.Array: Simplify implementation of length (by calling reserve).". Additionally, `Array`'s variadic constructor is now safer: If `emplace`-ing an element fails, successfully `emplace`-d elements are now destroyed. Example: ``` auto a = Array!S (S(0), S(1), S(2), S(3)); // If emplace-ing a[2] (as a copy of S(2)) fails: // - pre-existing behaviour: a[1] and a[0] were not destroyed (bad); // - new behaviour: a[1] and a[0] are destroyed (good). ``` A corresponding `unittest` has been added (`S.s_nDestroyed == S.s_nConstructed`): When such a failure (`Exception`) is caught, there should be no leaked objects. Implementation: Previously: - the `Payload` `length` used to be increased only once, after having successfully `emplace`-d all elements, thus incorrectly tracking the number of `emplace`-d elements during the loop (but correctly tracking it after the loop -- iff the entire loop succeeded). Now: - the `Payload` `length` is incremented multiple times, at each step i.e. after having successfully `emplace`-d each element, thus correctly tracking the number of `emplace`-d elements during the loop. Note: When the constructor fails, the corresponding destructor is not run. But the destructors for successfully constructed sub-objects are run; in this case, the destructor for the `_data` sub-object is run -- and it is this destructor which destroys the successfully emplaced elements (iff they have been correctly tracked).

edi33416 · 2021-10-18T16:41:34Z

std/container/array.d

-                GC.addRange(p, T.sizeof * values.length);
+            _data.refCountedStore.ensureInitialized();
+            _data.reserve(values.length);
+            foreach (ref value; values)


Imho, this can be simplified even further, by calling copyEmplace from core.lifetime.
This already takes exceptions into account and correctly unwinds an incomplete ctor call

Thank you for mentioning copyEmplace and linking to the relevant code. That has eased my learning attempt.

IMO:

And separately from whether implementation source code is simplified (a discussion which I would like to defer a little bit, if you kindly agree):

We are currently in a good position with the current approach (Payload.~this) and we should not move in the direction of copyEmplace.

Let us suppose that one of the emplacements throws an Exception and that application code (on that thread) does not catch it.

With both the current approach (Payload.~this) and the proposed approach (copyEmplace), the thread and the application are terminated and, at least on many Unix systems, if a debugger is not attached (and normally one is not -- for users running the application), a core file is dumped.

With the current approach, the core file (including the backtrace of the offending thread) is a snapshot of the process at the moment when the Exception is thrown in the constructor of the T element.

But with copyEmplace, the snapshot would capture a different, later, and less useful moment: the one when the Exception is rethrown within the body of copyEmplace (common code for all Ts and all T constructors and all T instances).

Therefore, the current approach is of greater help when debugging the core file (the crash).

I wish to mention that there is nothing wrong with crashing in such a case. With current technology at least, crashing is the way to produce rich debugging information (a core file). I should have probably written "when debugging/investigating the root cause of the failure".

This has been just my opinion. Corrections are warmly welcome.

An Exception contains information about the location from which it was first thrown, and that information is preserved when it is re-thrown from a different location. For example:

void fun() { try throw new Exception("oops"); // line 4 catch (Exception e) throw e; // line 6 } void main() { import std.stdio; try fun(); catch (Exception e) writeln("e was thrown from ", e.file, " at line ", e.line); // e was thrown from example.d at line 4 }

IMO: That is good, but not equal to what the core file captures (memory contents and registers for all threads) and not equal because of more elapsed time, different stack frame.

For example: Several stack frames of the point of the original throw are lost.

Let us suppose the failing constructor calls f which calls g twice which each calls h1 three times and h2 nine times.

In general, even if we recover the knowledge that h2 has thrown, it might be difficult to find out which of the eighteen calls to h2 has thrown (in absence of stack frames for h2 and g and f).

And we would not see stack frames (variables, arguments) for h2, g, f or the constructor.

And other pieces of memory and registers might be modified between the point-of-the-original-throw and the point-of-the-rethrow, e.g. by destruction of the successfully-constructed elements (which elements, by the way, might have been useful for investigations).

For example: Several stack frames of the point of the original throw are lost.

If you modify the example to print the full stack trace (e.info), you will see that the original stack trace is preserved as well.

I have currently put aside (if you kindly agree) a discussion of whether e.info can be printed offline (by the debugger from a post-mortem core file) or whether that is easy to do.

Such a discussion is relevant because one might not have already instrumented the code to print e.info.

Sometimes one cannot even if one wishes too -- the thread might be spawned/written by another programmer/team/vendor.

Other times, it may be too late and/or difficult to instrument the code, e.g. when many instances of the aplication have already been deployed to end users and all we are given are logs without the instrumentation -- but, I hope, also core files.

(I believe it is reasonable to expect that core files can be generated. This is a standard-and-widely-available mechanism which any user can enable. It does not require modification of the code of the application for which core files are to be dumped in case of a crash.)

(I admit that on many systems the dumping of core files is not enabled by default. In such cases, if the crash is difficult to reproduce, enabling core dumping may also be too late. But this is still easier and less-often-too-late than having to deploy a new version of the software--with new instrumentation which prints e.info.)

For example: Several stack frames of the point of the original throw are lost.

If you modify the example to print the full stack trace (e.info), you will see that the original stack trace is preserved as well.

In particular, there is a significant difference between seeing the names of the functions in the stack trace and seeing the stack variables and the arguments.

With optimizations, the latter is not always possible even in the presence of debugging information.

But, very often, debugging information gives more accurate/complete information for each stack frame: name of function, sometimes some of the arguments too, sometimes some of the variables too.

Without debugging information, we may not even see all the functions in the backtrace (e.g. because of inlining or function call tailing).

I wish to also tag @ibuclaw for expertise on "D and the GNU toolchain (including gdb)".

Unhandled exceptions are caught by the D runtime and cause the process to terminate by returning from its normal entry point with an exit code of EXIT_FAILURE. As far as I know this kind of termination does not generate a core file, but even if it did, the core file would not contain information about the context of the original throw because by the time it was generated, the stack would already have been unwound (and overwritten by the function calls to print the stack trace).

Unhandled exceptions are caught by the D runtime and cause the process to terminate by returning from its normal entry point with an exit code of EXIT_FAILURE. As far as I know this kind of termination does not generate a core file, but even if it did, the core file would not contain information about the context of the original throw because by the time it was generated, the stack would already have been unwound (and overwritten by the function calls to print the stack trace).

@pbackus: Thank you for all clarifications.

I also believe that exiting with whatever exit code does not generate a core file (https://en.wikipedia.org/wiki/Signal_(IPC)#Default_action).

Also, I believe that even if it did (e.g. if code in druntime is modified to raise a signal), the value of that core file would be even less (by a large margin) than our own temporary trapping of the exception in emplaceCopy.

DoctorNoobingstoneIPresume force-pushed the 210709_std_container_array_Payload_DefaultConstruct_01h branch 2 times, most recently from d0b1655 to 85d6f1c Compare July 10, 2021 12:45

dkorpel reviewed Jul 11, 2021

View reviewed changes

DoctorNoobingstoneIPresume force-pushed the 210709_std_container_array_Payload_DefaultConstruct_01h branch from 85d6f1c to 8cdfb5c Compare July 13, 2021 02:07

DoctorNoobingstoneIPresume force-pushed the 210709_std_container_array_Payload_DefaultConstruct_01h branch from 8cdfb5c to f1a0c56 Compare July 13, 2021 04:59

dkorpel reviewed Jul 13, 2021

View reviewed changes

DoctorNoobingstoneIPresume force-pushed the 210709_std_container_array_Payload_DefaultConstruct_01h branch from f1a0c56 to fcb473f Compare July 13, 2021 15:46

DoctorNoobingstoneIPresume force-pushed the 210709_std_container_array_Payload_DefaultConstruct_01h branch 2 times, most recently from 933b903 to c4a51fc Compare July 13, 2021 17:19

DoctorNoobingstoneIPresume force-pushed the 210709_std_container_array_Payload_DefaultConstruct_01h branch 3 times, most recently from d8c5205 to 0c96d4f Compare July 17, 2021 02:15

pbackus reviewed Aug 19, 2021

View reviewed changes

DoctorNoobingstoneIPresume force-pushed the 210709_std_container_array_Payload_DefaultConstruct_01h branch 3 times, most recently from 26ca49a to 810d155 Compare September 26, 2021 22:01

DoctorNoobingstoneIPresume force-pushed the 210709_std_container_array_Payload_DefaultConstruct_01h branch from 810d155 to 6b05400 Compare September 26, 2021 23:23

RazvanN7 approved these changes Oct 8, 2021

View reviewed changes

RazvanN7 merged commit 12b4380 into dlang:master Oct 8, 2021

edi33416 reviewed Oct 18, 2021

View reviewed changes

	import std.algorithm.comparison;
	import std.algorithm.comparison : equal;

		// We increment the length after each iteration (as opposed to adjusting it just once, after the loop)
		// in order to improve error-safety (in case one of the calls to emplace throws).

std.container.Array: Payload "default constructor" simplifies Array. #8162

std.container.Array: Payload "default constructor" simplifies Array. #8162

Conversation

DoctorNoobingstoneIPresume commented Jul 9, 2021

dlang-bot commented Jul 9, 2021

Bugzilla references

Testing this PR locally

dkorpel left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DoctorNoobingstoneIPresume Jul 13, 2021 • edited

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

DoctorNoobingstoneIPresume commented Jul 13, 2021 • edited

RazvanN7 commented Jul 13, 2021

Choose a reason for hiding this comment

DoctorNoobingstoneIPresume Jul 13, 2021 • edited

Choose a reason for hiding this comment

DoctorNoobingstoneIPresume commented Jul 13, 2021

DoctorNoobingstoneIPresume commented Jul 13, 2021 • edited

DoctorNoobingstoneIPresume commented Jul 13, 2021 • edited

Choose a reason for hiding this comment

DoctorNoobingstoneIPresume Sep 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DoctorNoobingstoneIPresume Sep 26, 2021 • edited

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

RazvanN7 commented Sep 13, 2021

DoctorNoobingstoneIPresume commented Sep 24, 2021 • edited

pbackus commented Sep 24, 2021

Choose a reason for hiding this comment

DoctorNoobingstoneIPresume Oct 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DoctorNoobingstoneIPresume Oct 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DoctorNoobingstoneIPresume Oct 26, 2021 • edited

Choose a reason for hiding this comment

DoctorNoobingstoneIPresume Oct 26, 2021 • edited

Choose a reason for hiding this comment

DoctorNoobingstoneIPresume Oct 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DoctorNoobingstoneIPresume Jul 13, 2021 •

edited

DoctorNoobingstoneIPresume commented Jul 13, 2021 •

edited

DoctorNoobingstoneIPresume Jul 13, 2021 •

edited

DoctorNoobingstoneIPresume commented Jul 13, 2021 •

edited

DoctorNoobingstoneIPresume commented Jul 13, 2021 •

edited

DoctorNoobingstoneIPresume Sep 24, 2021 •

edited

DoctorNoobingstoneIPresume Sep 26, 2021 •

edited

DoctorNoobingstoneIPresume commented Sep 24, 2021 •

edited

DoctorNoobingstoneIPresume Oct 25, 2021 •

edited

DoctorNoobingstoneIPresume Oct 25, 2021 •

edited

DoctorNoobingstoneIPresume Oct 26, 2021 •

edited

DoctorNoobingstoneIPresume Oct 26, 2021 •

edited

DoctorNoobingstoneIPresume Oct 26, 2021 •

edited