Skip to content

Conversation

@bobtista
Copy link

@bobtista bobtista commented Dec 6, 2025

Ran clang-tidy (readability-container-size-empty))

@bobtista bobtista force-pushed the bobtista/refactor/clang-tidy-container-empty branch from d87241f to 9174538 Compare December 6, 2025 19:02
@bobtista bobtista added Minor Severity: Minor < Major < Critical < Blocker Refactor Edits the code with insignificant behavior changes, is never user facing labels Dec 6, 2025
Copy link

@xezon xezon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement. With std::list from STLPort this can give small performance gains too.

@xezon xezon added the Performance Is a performance concern label Dec 7, 2025
@xezon xezon added this to the Code foundation build up milestone Dec 7, 2025
@xezon
Copy link

xezon commented Dec 9, 2025

Is this change complete or is it still missing checks from files it previously failed on?

@bobtista bobtista force-pushed the bobtista/refactor/clang-tidy-container-empty branch from 603abdc to 0226774 Compare December 10, 2025 16:17
@bobtista
Copy link
Author

Is this change complete or is it still missing checks from files it previously failed on?

I ran it again, it should have all the changes it's going to find for now.

@Caball009
Copy link

Caball009 commented Dec 12, 2025

If you search in Visual Studio with regex query [>\.] *size *\( *\) *.[>=] *0, you should still find a couple, including some size() > 0 cases.

@xezon
Copy link

xezon commented Dec 12, 2025

Maybe clang tidy is not good enough and we always need a manual pass as well.

@bobtista
Copy link
Author

I think there are lots of files that it skips, and some files require multiple runs. It takes me 30min+ on my windows machine to run it on the repo, though I try to do things like output the files it touches for a given check and then only rerun it on those files. I think in order to be 100% done with a check yes we should check manually after running the tool. After mfc is removed it will cover more files for example

@xezon
Copy link

xezon commented Dec 13, 2025

Ok then how about we do a clang-tidy run and then afterwards do another scripted or manual pass to fix whatever it has missed. So essentially it would be good to improve the same kind of thing across all files and not be at the mercy of clang-tidy to find it for us, but just help us kick it off.

As Caball009 has shown, we can regex search to find more occurences and then fix them.

@bobtista bobtista force-pushed the bobtista/refactor/clang-tidy-container-empty branch from e4b0502 to c0d4788 Compare December 13, 2025 21:48
@bobtista
Copy link
Author

Fixed the remaining ones I could find, rebased main

@bobtista bobtista force-pushed the bobtista/refactor/clang-tidy-container-empty branch from cc8874e to ee3bf58 Compare December 14, 2025 14:53
Copy link

@xezon xezon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks all correct. I did not look if this is complete.

@xezon xezon changed the title refactor: use container.empty() instead of container.size() == 0 refactor: Apply the readability-container-size-empty check with clang-tidy Dec 14, 2025
@Caball009
Copy link

I see zero remaining cases for that regex query, so I presume all cases have been taken care of.

@xezon xezon merged commit 746c017 into TheSuperHackers:main Dec 14, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Minor Severity: Minor < Major < Critical < Blocker Performance Is a performance concern Refactor Edits the code with insignificant behavior changes, is never user facing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants