Refactor the transformation class to accept dynamic arrays
mahussain jcmcdonald ardunster memateo
- Maniphest Tasks
- T1391: Refactor Transformation to transform static layers
Write test classes
|34 ↗||(On Diff #1636)|
That was probably because @gacmix was writing the copy assignment and referring to the previous version of Transformation and I saw the copy constructor and said he needed both.... Without realizing it was effectively the same as the constructor he already had with a Transformation as an argument 🤦♀️
@memateo You should Commandeer this revision. From the bottom of the page, go to Add Action... → Commandeer Revision. This will make you the author, and give you complete control over the diff, as if it were your own.
Was looking to see if this was ready to approve yet but there's no tests so I guess not? Probably just needs to pass similar tests as the original Transformation class. I know Graham had planned to delete the original class and replace it with this one once it was all in working order.
I'm going to go on a limb and suggest this happened as a result of random typing glitching during a liveshare session....
|1 ↗||(On Diff #1667)|
This should have version and library and license info at the top like all the other header files, copypaste with a few edits should be fine. The ones in SIMPLEXpress have an edit date, too, not sure what the standard is on that.
|6 ↗||(On Diff #1696)|
Our convention is to place names alphabetically by last name, principally to avoid debates about prominence of names.
|36 ↗||(On Diff #1696)|
I don't recommend naming a class "Refactored". Just replace the old version. We're using Git, so you can always see the old versions.
|45 ↗||(On Diff #1696)|
Can rows and columns ever be negative? If not, make the parameters unsigned int, or even better, size_t (an unsigned integer type used by most data structures for indices and lengths.)
|47 ↗||(On Diff #1696)|
If you make the above change, you'll also want to change the type here from int to size_t.
Additionally, I recommend using prepended increment/decrement anytime the return value from the operation is not directly used, as ++i is one less CPU instruction than i++. The compiler usually optimizes this out anyway, but it's good to be in the habit for the rare scenario when it doesn't.
|66 ↗||(On Diff #1696)|
You need a documentation comment on each function. Documentation comments are either /// ... or /** ... */. For normal functions (as opposed to constructors), you also need to describe all the parameters (\param) and the return value (\return), as appropriate.
|67–68 ↗||(On Diff #1696)|
Comments inside functions should be ordinary comments //, not doc comments.
|69 ↗||(On Diff #1696)|
Switch from spaces to tabs across all your files, please. Visual Studio Code can do this for you, and then clangformat can help with the rest of the formatting. Talk to @ardunster on how to do that.
|1 ↗||(On Diff #1667)|
Yeah, we dropped the edit date convention, but there are still files that haven't been updated accordingly.
I agree with bringing the heading and license comments over though!
I know this is only visible because of running format, but I suspect that what was intended here was luminance, not luminace, given that that's not a real word.
It seems to me you only need to clear through the lint errors before we can land this. I've left a few comments to help you there, but you'll want to look at the rest of the lint errors and fix them. The only error type you should be suppressing with // cppcheck-suppress is constStatement — NONE of the others!
Since this is syntactically correct with Eigen (apparently), you may need to silence the linter on this topic like this.
You can find the rest.
This is rather odd to me. What actually happens if you combine these lines together? (I know this wasn't your code.)
- Supressed a few errors, fixed some format, will need to look into layerinstance_tests
It looks like it purposefully sets up a dummy layer instance, and then changes it to be a different one to test the copy constructor and make sure it works properly. Should I still change it?
- Fixed luminace typo in colors.cpp
- Getting weird error pretaining to IoChannel and Goldilocks, fixed a few other things blocking the make
I feel like this is worth landing if the tests are passing (as they apparently are). Further improvements can be made as needed, especially away from the formatting fixes.
Also, just checking, the contents of your .clang-format should be the same as in P56.
@memateo I don't want to lose all this work. What would it take to land this? Does it just need a review and approval from me?