InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.43k stars 668 forks source link

PERF: readability container size empty #4985

Closed hjmjohnson closed 4 days ago

hjmjohnson commented 5 days ago

The emptiness of a container should be checked using the empty() method instead of the size() method. It is not guaranteed that size() is a constant-time function, and it is generally more efficient and also shows clearer intent to use empty(). Furthermore some containers may implement the empty() method but not implement the size() method. Using empty() whenever possible makes it easier to switch to another container in the future.

cd ${BLDDIR} run-clang-tidy.py -extra-arg=-Dclang -checks=-,readability-container-size-empty -header-filter=. -fix

N-Dekker commented 5 days ago

It is not guaranteed that size() is a constant-time function

Actually, it is. For all std container types, size() has a constant-time complexity. So I don't expect any PERF improvement from this pull request. It's just a STYLE improvement to me. But I like it anyway 👍


Originally, Microsoft implemented empty() by calling size(), so empty() might then even be slower than size() 😱 Fortunately I was able to convince them not to do so anymore 😇: https://github.com/microsoft/STL/pull/1836

hjmjohnson commented 5 days ago

It is not guaranteed that size() is a constant-time function

Actually, it is. For all std container types, size() has a constant-time complexity. So I don't expect any PERF improvement from this pull request. It's just a STYLE improvement to me. But I like it anyway 👍

Originally, Microsoft implemented empty() by calling size(), so empty() might then even be slower than size() 😱 Fortunately I was able to convince them not to do so anymore 😇: microsoft/STL#1836

FYI: The message is copied from the clang-tidy documentation page. I took it on faith that the documentation was correct. i trust your experiences more than the documentation :).

N-Dekker commented 4 days ago

The message is copied from the clang-tidy documentation page. I took it on faith that the documentation was correct. i trust your experiences more than the documentation :).

Ah, thanks Hans, I see:

Maybe I should ask them for a clarification 🤔

N-Dekker commented 4 days ago

@dzenanz @hjmjohnson Please "like" (👍) my clang-tidy pull request on this topic https://github.com/llvm/llvm-project/pull/117629 It is inspired by your pull request, Hans 😃 !

dzenanz commented 4 days ago

Done. I am not sure how much weight my approving review will carry 😄