Open FederAndInk opened 3 years ago
Also, why are you deleting move ctor/operator= of subdimension, I understand that it's just a proxy, but as it is just spans it will do the same as copy, and why not defaulting: https://github.com/cristi1990an/Tensor-plus-plus/blob/25de86a8068bcbe6d7fde8f4a791c872301ce8ab/tensor.hpp#L724
I don't think those are meaningful operations (see: https://en.cppreference.com/w/cpp/named_req/RandomAccessIterator): https://github.com/cristi1990an/Tensor-plus-plus/blob/25de86a8068bcbe6d7fde8f4a791c872301ce8ab/tensor.hpp#L1192 https://github.com/cristi1990an/Tensor-plus-plus/blob/25de86a8068bcbe6d7fde8f4a791c872301ce8ab/tensor.hpp#L1335
here you can use []
: https://github.com/cristi1990an/Tensor-plus-plus/blob/25de86a8068bcbe6d7fde8f4a791c872301ce8ab/tensor.hpp#L1297
why using temporary variables? just do return const_iterator(ptr++)
: https://github.com/cristi1990an/Tensor-plus-plus/blob/25de86a8068bcbe6d7fde8f4a791c872301ce8ab/tensor.hpp#L1265 also on other methods in iterator and const_iterator
and in you iterators, you could factorize code duplication by calling similar functions like with offset + it
and it + offset
just implement once and in the other call it by swapping values
These are all amazing points, thank you so much for contributing to this! I'll start looking into each of them!
On Wed, Aug 18, 2021, 01:34 Ludovic J @.***> wrote:
From your comment on #3 https://github.com/cristi1990an/Tensor-plus-plus/pull/3 here are some suggestions, Project layout
- please provide a .clang-format file into the repo and use clang-format, it will help contribution
- organize your files, make tests/src/include directories
- use more tools, like sanitizers/clang-tidy...
- you might want to use a testing library (e.g.: boost test, google test, ...)
- watch out for your repo, there is a lot of duplicated commits, and strange merge from master, maybe get a better understanding of how Git works: https://git-scm.com/book/en/v2
- you can use git revert for some or git reset/git commit --amend if it's not pushed
- you should use a better name for 'helpers.hpp' as it is just IO (generally avoid generic names like helper, utils, controller...)
On the code itself:
- you are using redundant == true and == false (clang-tidy is giving me this warning btw, you'd have to activate readability checks)
- be consistent on the use of ! or not pick one (! is more common)
- I am not a big fan of loops and ifs without blocks {} to avoid problems and be consistent
- param_1 You might want to name those (and also TEST_1...)
- generally avoid using raw new/delete it's not exception safe, etc, and in this case I think you could use std::swap_ranges, and why are you making copies? https://github.com/cristi1990an/Tensor-plus-plus/blob/5c902acefbc6e07b47f9555294e20677cf8d34e6/tensor.hpp#L1275
- also, you could factorize your duplicated code, like for instance, call the lvalue swap simply like swap(left, right) in the rvalue overload, it works because the rvalue has been named in the function: https://github.com/cristi1990an/Tensor-plus-plus/blob/25de86a8068bcbe6d7fde8f4a791c872301ce8ab/tensor.hpp#L1388
- if you haven't watched it already, here is a great talk on standard algorithms: https://www.youtube.com/watch?v=2olsGf6JIkU, generally CppCon is great and freely available on youtube :)
- avoid using reinterpret_cast and read carefully the rules if you don't know them already (it's always good to reread them: https://en.cppreference.com/w/cpp/language/reinterpret_cast), moreover here you could just call _data.data() or to be more generic std::data(_data): https://github.com/cristi1990an/Tensor-plus-plus/blob/25de86a8068bcbe6d7fde8f4a791c872301ce8ab/tensor.hpp#L1074
- if you want to be more generic, you should prefer free function for begin/end/data/size/... (std::begin/std::end/... instead of methods)
- If you use C++20, you might want to define more operator<=> you could also default them
- swap and move operations should not throw
- I think you might want to follow the rule of 0 for your iterators
- you should use std::equal with 4 iterators instead of 3 to do bound checking (I see that by design they have the same size, you should put comments at least then, there is also == it should work, maybe create a span with a templated size) minor issue
- This would probably better be some kind of assert, this seems to be a logic error: https://github.com/cristi1990an/Tensor-plus-plus/blob/8983300155496ab2acd3b143ac88dec3d568c634/tensor.hpp#L1326
- Choose to use (void) or () and stay consistent: https://github.com/cristi1990an/Tensor-plus-plus/blob/8983300155496ab2acd3b143ac88dec3d568c634/tensor.hpp#L1070
I might not have understood all your math/thinking/vision. Also I haven't done a detailed review.
Great work, Hope that'll help :)
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/cristi1990an/Tensor-plus-plus/issues/19, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFSQZMCG66DUPXWOSH3WKLTT5LPVVANCNFSM5CKZLTGQ .
Any idea how am I supposed to modify "CMakeLists.txt" for this PR? https://github.com/cristi1990an/Tensor-plus-plus/pull/24
From your comment on #3 here are some suggestions,
Project layout
.clang-format
file into the repo and use clang-format, it will help contributiongit revert
for some orgit reset
/git commit --amend
if it's not pushedOn the code itself:
== true
and== false
(clang-tidy is giving me this warning btw, you'd have to activate readability checks)!
ornot
pick one (!
is more common){}
to avoid problems and be consistentparam_1
You might want to name those (and also TEST_1...)swap(left, right)
in the rvalue overload, it works because the rvalue has been named in the function: https://github.com/cristi1990an/Tensor-plus-plus/blob/25de86a8068bcbe6d7fde8f4a791c872301ce8ab/tensor.hpp#L1388_data.data()
or to be more genericstd::data(_data)
: https://github.com/cristi1990an/Tensor-plus-plus/blob/25de86a8068bcbe6d7fde8f4a791c872301ce8ab/tensor.hpp#L1074==
it should work, maybe create a span with a templated size) minor issue(void)
or()
and stay consistent: https://github.com/cristi1990an/Tensor-plus-plus/blob/8983300155496ab2acd3b143ac88dec3d568c634/tensor.hpp#L1070I might not have understood all your math/thinking/vision. Also I haven't done a detailed review.
Great work, Hope that'll help :)