[clang-tidy] readability-container-size-empty suggests incorrect replacement

[aij]

I encountered this in a large codebase. Running run-clang-tidy-20 -checks='-*,readability-container-size-empty' -p . -fix resulted in code that no longer compiled.

The cases I noticed involved a subclass of a std container where size() was being used in a member function.

Unexpected behavior: !size() replaced by size->empty() and size() >= 1 was replaced by !size->empty()

Expected behavior: !size() should be replaced by empty() and size() >= 1 should be replaced by !empty(). (Alternatively, this->empty() and !this->empty() would also be acceptable.)

I have attached a minimal repro where the incorrect replacement can be seen with

$ clang-tidy -checks='-*,readability-container-size-empty' break-readability-container-size-empty.c++
Error while trying to load a compilation database:
Could not auto-detect compilation database for file "break-readability-container-size-empty.c++"
No compilation database found in /home/ivan/git/bug-readability-container-size-empty or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
1 warning generated.
/home/ivan/git/bug-readability-container-size-empty/break-readability-container-size-empty.c++:10:10: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
   10 |     if (!size())
      |         ~^~~~~~
      |         size->empty()
/nix/store/qnwxpk0in4bm43q2qnykvkjxa9qhqd0z-gcc-14.3.0/include/c++/14.3.0/bits/basic_string.h:1232:7: note: method 'basic_string<char>'::empty() defined here
 1232 |       empty() const _GLIBCXX_NOEXCEPT
      |       ^

break-readability-container-size-empty.c++

The same can also be seen on Godbolt: https://godbolt.org/z/jx4xWscdj

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Ivan Jager (aij)

I encountered this in a large codebase. Running `run-clang-tidy-20 -checks='-*,readability-container-size-empty' -p . -fix` resulted in code that no longer compiled.

The cases I noticed involved a subclass of a std container where size() was being used in a member function.

Unexpected behavior: !size() replaced by size->empty() and size() >= 1 was replaced by !size->empty()

Expected behavior: !size() should be replaced by empty() and size() >= 1 should be replaced by !empty(). (Alternatively, this->empty() and !this->empty() would also be acceptable.)

I have attached a minimal repro where the incorrect replacement can be seen with

$ clang-tidy -checks='-*,readability-container-size-empty' break-readability-container-size-empty.c++
Error while trying to load a compilation database:
Could not auto-detect compilation database for file "break-readability-container-size-empty.c++"
No compilation database found in /home/ivan/git/bug-readability-container-size-empty or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
1 warning generated.
/home/ivan/git/bug-readability-container-size-empty/break-readability-container-size-empty.c++:10:10: warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]
   10 |     if (!size())
      |         ~^~~~~~
      |         size->empty()
/nix/store/qnwxpk0in4bm43q2qnykvkjxa9qhqd0z-gcc-14.3.0/include/c++/14.3.0/bits/basic_string.h:1232:7: note: method 'basic_string<char>'::empty() defined here
 1232 |       empty() const _GLIBCXX_NOEXCEPT
      |       ^

break-readability-container-size-empty.c++

The same can also be seen on Godbolt: https://godbolt.org/z/jx4xWscdj

[LegalizeAdulthood]

The cases I noticed involved a subclass of a std container

While the check shouldn't produce incorrect fixits in this case, you do know that subclassing standard containers is not a good idea?

[aij]

do know that subclassing standard containers is not a good idea?

I do. This was encountered on a legacy codebase.

I also just noticed in one case it happened the code was not subclassing a standard container. My initial attempt to produce a minimal repro without subclassing a standard container didn't work (clang-tidy didn't warn at all) but I may give it another try.

[flovent]

I also just noticed in one case it happened the code was not subclassing a standard container. My initial attempt to produce a minimal repro without subclassing a standard container didn't work (clang-tidy didn't warn at all) but I may give it another try.

An example without standard container, maybe you forget making methods const? that's what i ignore at the beginning.

void bar();

class Container {
public:
    int size() const;
    bool empty() const;
};

class foo : public Container {

  void doit() {
    if (!size())
      bar();
  }
};

https://godbolt.org/z/nnnG7aMKs

[vbvictor]

Created a separate issue to detect calls to regular member functions (without any need to subclass) #152649

[aij]

maybe you forget making methods const?

My first attempt was without subclassing, so I guess I hit #152649 , but it wasn't clear to me that that was why it didn't apply.