ngnrsaa / qflex

Flexible Quantum Circuit Simulator (qFlex) implements an efficient tensor network, CPU-based simulator of large quantum circuits.
Apache License 2.0
97 stars 24 forks source link

Improve C++ code. #225

Closed s-mandra closed 4 years ago

s-mandra commented 4 years ago

Should fix #224.

s-mandra commented 4 years ago

I've added -Wall and -Wpedantic to improve the c++ code.

95-martin-orion commented 4 years ago

See comment on issue #224 - and again, apologies for only getting to this after the fact.

s-mandra commented 4 years ago

See comment on issue #224 - and again, apologies for only getting to this after the fact.

As I wrote in #224, here I disagree. this issue is beyond LGTM but clarification of intents. ints shouldn't be used freely if std::size_t are more appropriate. For instance, when accessing an array, it's more appropriate to use std::size_t because it guarantees that std::size_t will be large enough to cover the addressable memory. Large part of the conversions were related to that. There are no advantages (in terms of performance) to have int rather than std:size_t. Moreover, having std::size_t helps to early catch errors related to signed conversion that may cause overflow.

Also, I've already made all the conversions :) Everything works properly but there is a loop in fast_reordering that has been never checked so far neither with the old implementation nor with my int-->std::size_t conversion.

s-mandra commented 4 years ago

Finally, as a plus, avoiding avoidable conversions using std::size_t allowed me to catch all warnings with -Wall -Wpedantic to have a more portable code. At the moment, the code is warning free and I'm even using -Werror to treat every warning as an error ;) (useful when you want to compile your code using different compilers)

s-mandra commented 4 years ago

I'm happy with this PR. @95-martin-orion, let me know if you spot something else. Thanks for your help!

alankao64 commented 4 years ago

Confirmed these changes do not break any tests in master or in other existing branches.