Apply Clang-Tidy and VS static analysis suggestions. #78957

CLIDragon · 2025-01-05T04:22:21Z

Summary

Infrastructure "Apply Clang-Tidy and VS static analysis suggestions."

Purpose of change

Improve code readability and performance. On my machine this reduced mean time per do_turn from 135 microseconds to 131.5, around a 2.5% speedup.

Describe the solution

Apply changes. There are a lot of changes but they're grouped by commit and nearly all changes are trivial.

Describe alternatives you've considered

None.

Testing

The game compiles and tests run.

Additional context

github-actions

Auto-requesting reviews from non-collaborators: @wapcaplet @Qrox @andrei8l

moxian · 2025-01-05T04:39:20Z

Do you know why are these issues not caught by our current clang-tidy CI setup? Is it a window-specific issue, a newer clang-tidy version that you are using or something else?

CLIDragon · 2025-01-05T09:34:45Z

Do you know why are these issues not caught by our current clang-tidy CI setup? Is it a window-specific issue, a newer clang-tidy version that you are using or something else?

For the commits that don't have clang-tidy in them, they were caught by ReSharper or VS not clang-tidy. For the clang-tidy specific issues, I think it's just that we don't have those checks enabled.

moxian · 2025-01-05T18:51:33Z

For the clang-tidy specific issues, I think it's just that we don't have those checks enabled.

We should
We have enabled all performance-* checks except performance-avoid-endl, performance-noexcept-swap, performance-no-automatic-move.
Why do we not catch performance-enum-size, performance-unnecessary-copy-initialization, performance-unnecessary-value-param, performance-move-const-arg?

What is your clang-tidy version? How do you obtain the binary?

The performance-noexcept-swap and performance-no-automatic-move that you fix were "temporarily" disabled in #71721, so the fix is good, but in that case they should probably be un-blacklisted from .clang-tidy config.

(separately, the CI is failing since you probably want to add #include <cstdint> for files with new enum sizes)

andrei8l · 2025-01-05T19:17:43Z

Why do we not catch performance-enum-size, performance-unnecessary-copy-initialization, performance-unnecessary-value-param, performance-move-const-arg?

performance-enum-size was added in version 18 and we use 17. The quality of the other checks was improved in version 19.

I don't think it makes much sense to address these unless our CI version detects them or they cause significant issues. performance-enum-size in particular is little more than churn.

CLIDragon · 2025-01-06T02:06:48Z

I don't think it makes much sense to address these unless our CI version detects them or they cause significant issues. performance-enum-size in particular is little more than churn.

performance-unnecessary-copy-initialization, performance-unnecessary-value-param, and performance-move-const-arg all have genuine performance benefits. #78945 is a recent example but there are certainly more. performance-enum-size only causes churn because it has never been addressed in any kind of systematic manner. If it were integrated into the CI it would be a barely noticeable change which may improve performance and will definitely reduce memory usage.

* performance-unnecessary-copy-initialization * performance-unnecessary-value-param * performance-move-const-arg * readability-container-size-empty

CLIDragon · 2025-01-06T13:44:07Z

clang-tidy is failing because padding on spell_type is 34 instead of 2 bytes. Given that the size of spell_type is 11544 bytes, I don't think 32 bytes is going to make a huge difference so I am leaning towards ignoring it.

Brambor · 2025-01-06T18:57:20Z

src/overmapbuffer.cpp

@@ -1251,7 +1251,7 @@ tripoint_abs_omt overmapbuffer::find_random(
    return find_random( origin, params );
 }

-shared_ptr_fast<npc> overmapbuffer::find_npc( character_id id )
+shared_ptr_fast<npc> overmapbuffer::find_npc( character_id id ) const
 {
    for( auto &it : overmaps ) {


I assume you will fix these (see build errors). Commenting so that it does not fly under the radar.

Brambor · 2025-01-06T19:02:44Z

I like the padding warnings. They taught us that in school but I never knew you could turn on a warning! (And never knew why you couldn't turn it on.)

@moxian Is setting up clang on a local machine manageable these days? can MSVC do it for a single file without compiling the whole project for like 6 hours?

moxian · 2025-01-06T20:24:31Z

Is setting up clang on a local machine manageable these days? can MSVC do it for a single file without compiling the whole project for like 6 hours?

Stock clang-tidy definitely is, but I haven't managed to build our cata plugin still.

I'm away from my proper PC for the next couple of days so I can't really give a proper answer but the short of it is:

grab clang-tidy exe from wherever (perhaps build from source, it's not too bad)
create compilation database
one way is to install clang power tools VS plugin. Which, actually, can just analyze individual files (which is probably the thing you actually want).. But it can also export compilation database at a click of a button (i didn't like the plugin tho, so I went a bit further down this rabbit hole)
the other way is

- cd build; cmake -G ninja -DDCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_TOOLCHAIN_FILE=C:/vcpkg/scripts/buildsystems/vcpkg.cmake -DTILES=1 -DSOUNDS=1 -DLOCALIZE=1 .. .
- Then keep feeding it extra vars it asks for (the paths to your clang and clang++ and llvm-rc and ninja (you can replace the -G Ninja with -G "unix makefiles" and use make there instead)
- then keep doingvcpkg install sdl2 and similar in your vcpkg directory (so C:/vcpkg/) as it keeps complaining
- - there is definitely a better way to do this with vcpkg manifests but it had some subtleties to it and idr the specifics right now and have no way to test
now that you have build/compile_commands.json you can feed it to clang tidy . Either move it to the root dir (i.e. mv build/compile_commands.json .) or give it to clang-tidy via -p build flag
invoke the thing on a single file: clang-tidy -p build src/achievements.cpp

CLIDragon · 2025-01-06T23:49:19Z

I missed it before, but widget is also failing due to excessive padding and cube_direction needs to include <cstdint>

akrieger · 2025-01-17T20:14:07Z

Can you separate out the enum class changes to a separate PR because they're the bulk of this change and probably the least interesting parts (and makes it harder to see what may be functionally contributing to the performance wins)?

akrieger · 2025-01-17T20:16:14Z

src/activity_actor.cpp

@@ -5730,7 +5730,7 @@ void outfit_swap_actor::finish( player_activity &act, Character &who )
    // Taken-off items are put in this temporary list, then naturally deleted from the world when the function returns.
    std::list<item> it_list;
    for( item_location &worn_item : who.get_visible_worn_items() ) {
-        item outfit_component( *worn_item );
+        const item &outfit_component( *worn_item );


Is this safe? does taking off the worn item not invalidate the item location and then invalidate the outfit component?

github-actions bot requested review from dseguin and KorGgenT January 5, 2025 04:22

github-actions bot reviewed Jan 5, 2025

View reviewed changes

github-actions bot added Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Jan 5, 2025

CLIDragon force-pushed the clang-tidy branch from 0d85281 to 07fa229 Compare January 5, 2025 04:29

CLIDragon added 13 commits January 6, 2025 13:28

clang-tidy

f16789f

* performance-unnecessary-copy-initialization * performance-unnecessary-value-param * performance-move-const-arg * readability-container-size-empty

clang-tidy : performance-no-automatic-move

12c02a2

clang-tidy : readability-reference-to-constructed-temporary

69565d0

Replace macros in cata_flat_set and make swap noexcept.

1ed6a9c

clang-tidy : performance-enum-size

294851d

clang-tidy: bugprone-optional-value-conversion

52e0077

clang-tidy : performance-no-automatic-move

ce330b3

clang-tidy : performance-enum-size

03db3ea

pass value parameters by const reference

c9e0a27

member function can be made const

6bbf01e

add noexcept to std::hash implementations

28e8dad

Apply patch to list from move to C++17.

14664ae

fix test failures

cb62e04

CLIDragon force-pushed the clang-tidy branch from 82b46ae to cb62e04 Compare January 6, 2025 03:28

CLIDragon closed this Jan 6, 2025

CLIDragon reopened this Jan 6, 2025

Brambor reviewed Jan 6, 2025

View reviewed changes

akrieger reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply Clang-Tidy and VS static analysis suggestions. #78957

Apply Clang-Tidy and VS static analysis suggestions. #78957

CLIDragon commented Jan 5, 2025

github-actions bot left a comment

moxian commented Jan 5, 2025

CLIDragon commented Jan 5, 2025

moxian commented Jan 5, 2025

andrei8l commented Jan 5, 2025

CLIDragon commented Jan 6, 2025

CLIDragon commented Jan 6, 2025 •

edited

Loading

Brambor Jan 6, 2025 •

edited

Loading

Brambor commented Jan 6, 2025 •

edited

Loading

moxian commented Jan 6, 2025 •

edited

Loading

CLIDragon commented Jan 6, 2025 •

edited

Loading

akrieger commented Jan 17, 2025

akrieger Jan 17, 2025

Apply Clang-Tidy and VS static analysis suggestions. #78957

Are you sure you want to change the base?

Apply Clang-Tidy and VS static analysis suggestions. #78957

Conversation

CLIDragon commented Jan 5, 2025

Summary

Purpose of change

Describe the solution

Describe alternatives you've considered

Testing

Additional context

github-actions bot left a comment

Choose a reason for hiding this comment

moxian commented Jan 5, 2025

CLIDragon commented Jan 5, 2025

moxian commented Jan 5, 2025

andrei8l commented Jan 5, 2025

CLIDragon commented Jan 6, 2025

CLIDragon commented Jan 6, 2025 • edited Loading

Brambor Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Brambor commented Jan 6, 2025 • edited Loading

moxian commented Jan 6, 2025 • edited Loading

CLIDragon commented Jan 6, 2025 • edited Loading

akrieger commented Jan 17, 2025

akrieger Jan 17, 2025

Choose a reason for hiding this comment

CLIDragon commented Jan 6, 2025 •

edited

Loading

Brambor Jan 6, 2025 •

edited

Loading

Brambor commented Jan 6, 2025 •

edited

Loading

moxian commented Jan 6, 2025 •

edited

Loading

CLIDragon commented Jan 6, 2025 •

edited

Loading