Closed s-mandra closed 4 years ago
There are a couple of places where we might want to use std::size_t
(e.g., for tensor dimensions that might exceed 2^32), but I don't think int
vs std::size_t
is the cause of the quality issue - see these docs. It should be enough to cast one of the operands to long
before the operation.
Sorry to make this observation after you already made all of the conversions :(
There are a couple of places where we might want to use
std::size_t
(e.g., for tensor dimensions that might exceed 2^32), but I don't thinkint
vsstd::size_t
is the cause of the quality issue - see these docs. It should be enough to cast one of the operands tolong
before the operation.Sorry to make this observation after you already made all of the conversions :(
Here I disagree. It's not just about LGTM but clarification of intents. int
s 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.
Also, I've already made the changes :)
After reading more about the advantages of std::size_t
vs. int
, I can see your point. size_t
probably isn't necessary everywhere (e.g. the i
in a for-loop that never gets compared to a size_t
), but from a development perspective it's easier just to use the same type throughout.
Something we might consider is documenting these style decisions as we go - that way, we'll have a guide to refer back to for future reviews.
After reading more about the advantages of
std::size_t
vs.int
, I can see your point.size_t
probably isn't necessary everywhere (e.g. thei
in a for-loop that never gets compared to asize_t
), but from a development perspective it's easier just to use the same type throughout.Something we might consider is documenting these style decisions as we go - that way, we'll have a guide to refer back to for future reviews.
Agreed. I changed int
--> std::size_t
wherever it was clear that the corresponding variable should have been a size, and changed int
--> long int
everywhere else (few cases, like while getting indexes from strings: in this case, user could specify -1
and overload to the largest std::size_t
).
We got a
C
aboutc++
quality because of potential overloads ofint
s. We should move fromint
s tostd::size_t
.