Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Upgrade clang-tidy to LLVM 18, and fix (some) newly firing lints (Cle…
…verRaven#79633) * Upgrade clang-tidy to llvm-18 - fix compilation issues - update the CI - drive-by change: bump the displayed "most time consuming checks" from top 10 to top 30 * Make .clang-tidy use structured list for checks instead of a single string that relies on `\` with no trailing whitespace for line joining. Both because the \ thing is bugprone, but mostly because i want to add extra comments about *why* the checks are disabled * Shuffle comments around to be closer to the thing they are commenting on * Disable newly discovered firing checks This partially defeats the whole point of update, but maybe that's still worth it. Improved existing checks: modernize-make-shared readability-redundant-member-init Newly introduced checks: bugprone-chained-comparison bugprone-multi-level-implicit-pointer-conversion bugprone-optional-value-conversion bugprone-unused-local-non-trivial-variable clang-analyzer-optin.core.EnumCastOutOfRange performance-enum-size readability-avoid-nested-conditional-operator readability-avoid-return-with-void-value readability-redundant-casting readability-redundant-inline-specifier readability-reference-to-constructed-temporary * Reenable and fix modernize-make-shared https://clang.llvm.org/extra/clang-tidy/checks/modernize/make-shared.html the new check is the x.reset(new T(args)) => x=make_shared::<T>(args) form example error: ##[error]/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/item_location.cpp:838:13: error: use std::make_shared instead [modernize-make-shared,-warnings-as-errors] 9 | ptr.reset( new impl::item_on_person( who_id, idx ) ); | ~^~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~ ~ | = std::make_shared<impl::item_on_person> * Re-enable and fix readability-redundant-member-init lint https://clang.llvm.org/extra/clang-tidy/checks/readability/redundant-member-init.html Sample error: ##[error]/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/effect_source.h:47:41: error: initializer for member 'fac' is redundant [readability-redundant-member-init,-warnings-as-errors] 47 | std::optional<faction_id> fac = faction_id(); | ~~^~~~~~~~~~~~ * Enable and fix bugprone-optional-value-conversion lint https://clang.llvm.org/extra/clang-tidy/checks/bugprone/optional-value-conversion.html Sample error: ##[error]/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/mission_util.cpp:182:67: error: conversion from 'std::optional<abstract_var_info<std::basic_string<char>>>' into 'abstract_var_info<std::basic_string<char>>' and back into 'std::optional<abstract_var_info<std::basic_string<char>>>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion,-warnings-as-errors] 182 | return project_to<coords::omt>( get_tripoint_ms_from_var( params.target_var.value(), d, false ) ); | ^ /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/mission_util.cpp:182:85: note: remove call to 'value' to silence this warning 182 | return project_to<coords::omt>( get_tripoint_ms_from_var( params.target_var.value(), d, false ) ); | ~^~~~~~~ * Fix and reenable bugprone-chained-comparison https://clang.llvm.org/extra/clang-tidy/checks/bugprone/chained-comparison.html It only crops up in tests due to how catch2 works. Catch2 has silenced the lint on their end in catchorg/Catch2@1078e7e This commit mirrors the upstream change Example error: ##[error]/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/zones_custom_test.cpp:56:9: error: chained comparison 'v0 <= v1 == v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison,-warnings-as-errors] 56 | REQUIRE( zmgr.get_near_zone_type_for_item( hammer, where ) == zone_type_LOOT_CUSTOM ); | ^ /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/catch/catch.hpp:17636:24: note: expanded from macro 'REQUIRE' 17636 | #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__ ) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/catch/catch.hpp:2706:47: note: expanded from macro 'INTERNAL_CATCH_TEST' 2706 | catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/zones_custom_test.cpp:56:9: note: operand 'v0' is here 56 | REQUIRE( zmgr.get_near_zone_type_for_item( hammer, where ) == zone_type_LOOT_CUSTOM ); | ^ /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/catch/catch.hpp:17636:24: note: expanded from macro 'REQUIRE' 17636 | #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__ ) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/catch/catch.hpp:2706:47: note: expanded from macro 'INTERNAL_CATCH_TEST' 2706 | catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); \ | ^~~~~~~~~~~~~~~~~~~ /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/zones_custom_test.cpp:56:18: note: operand 'v1' is here 56 | REQUIRE( zmgr.get_near_zone_type_for_item( hammer, where ) == zone_type_LOOT_CUSTOM ); | ^ /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/catch/catch.hpp:17636:90: note: expanded from macro 'REQUIRE' 17636 | #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__ ) | ^~~~~~~~~~~ /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/catch/catch.hpp:2706:70: note: expanded from macro 'INTERNAL_CATCH_TEST' 2706 | catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); \ | ^~~~~~~~~~~ /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/zones_custom_test.cpp:56:71: note: operand 'v2' is here 56 | REQUIRE( zmgr.get_near_zone_type_for_item( hammer, where ) == zone_type_LOOT_CUSTOM ); | ^ /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/catch/catch.hpp:17636:90: note: expanded from macro 'REQUIRE' 17636 | #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__ ) | ^~~~~~~~~~~ /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/tests/catch/catch.hpp:2706:70: note: expanded from macro 'INTERNAL_CATCH_TEST' 2706 | catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); \ | ^~~~~~~~~~~ * Fix and re-enable readability-reference-to-constructed-temporary https://clang.llvm.org/extra/clang-tidy/checks/readability/reference-to-constructed-temporary.html Example error: ##[error]/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/melee.cpp:1809:32: error: reference variable 'dest' extends the lifetime of a just-constructed temporary object 'const tripoint_bub_ms' (aka 'const coords::coord_point_ob<tripoint, coords::origin::reality_bubble, coords::scale::map_square>'), consider changing reference to value [readability-reference-to-constructed-temporary,-warnings-as-errors] 1809 | const tripoint_bub_ms &dest{ new_, b.z()}; | * Fix and reenable readability-avoid-return-with-void-value https://clang.llvm.org/extra/clang-tidy/checks/readability/avoid-return-with-void-value.html Example error: ##[error]/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/map.h:2397:13: error: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value,-warnings-as-errors] 2397 | return map::i_clear( rebase_bub( p ) ); | * Triage readability-redundant-inline-specifier as not desirable https://clang.llvm.org/extra/clang-tidy/checks/readability/redundant-inline-specifier.html Sample error: ##[error]/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/point.h:33:5: error: function 'is_invalid' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier,-warnings-as-errors] 33 | inline bool is_invalid() const { | ^~~~~~ Reasoning to keep disabled in the comments * Triage clang-analyzer-optin.core.EnumCastOutOfRange as not desirable https://clang.llvm.org/extra/clang-tidy/checks/clang-analyzer/optin.core.EnumCastOutOfRange.html Rationale in comments. Example error: ##[error]/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/enum_traits.h:99:12: error: The value '7' provided to the cast expression is not in the valid range of values for 'part_status_flag' [clang-analyzer-optin.core.EnumCastOutOfRange,-warnings-as-errors] 99 | return static_cast<E>( static_cast<I>( l ) | static_cast<I>( r ) ); | ^ /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/vehicle.h:100:12: note: enum declared here 100 | enum class part_status_flag : int { | ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~ 101 | any = 0, | ~~~~~~~~ 102 | working = 1 << 0, | ~~~~~~~~~~~~~~~~~ 103 | available = 1 << 1, | ~~~~~~~~~~~~~~~~~~~ 104 | enabled = 1 << 2 | ~~~~~~~~~~~~~~~~ 105 | }; | ~ /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/vehicle.cpp:3124:44: note: Calling 'operator|<part_status_flag, void>' 3124 | static_cast<part_status_flag>( part_status_flag::enabled | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ 3125 | part_status_flag::working | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~ 3126 | part_status_flag::available ) ); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/enum_traits.h:99:12: note: The value '7' provided to the cast expression is not in the valid range of values for 'part_status_flag' 99 | return static_cast<E>( static_cast<I>( l ) | static_cast<I>( r ) ); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ * Mark the remaining lints as "maybe some other day" Lints in question: bugprone-multi-level-implicit-pointer-conversion bugprone-unused-local-non-trivial-variable performance-enum-size readability-avoid-nested-conditional-operator readability-redundant-casting They look reasonable but I can't really deal with them right now. * Fix readability-container-size-empty (and keep it enabled) https://clang.llvm.org/extra/clang-tidy/checks/readability/container-size-empty.html Error: /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/climbing.cpp:222:13: error: the 'empty' method should be used to check for emptiness instead of 'length' [readability-container-size-empty,-warnings-as-errors] 222 | if( menu_hotkey_str.length() ) { | ^~~~~~~~~~~~~~~~~~~~~~~~ | !menu_hotkey_str.empty() * Disable two lints for performance reasons. Lints in question: misc-unused-using-decls modernize-replace-auto-ptr These are in top-10 most expensive checks. For context: here's the profiling data when running clang-tidy on ~most of the codebase (as seen in the CI in one of the versions of the current PR): { "time.clang-tidy.misc-unused-using-decls.wall": 1094.7915439605713, "time.clang-tidy.bugprone-stringview-nullptr.wall": 866.0817050933838, "time.clang-tidy.cata-redundant-parentheses.wall": 839.2004644870758, "time.clang-tidy.modernize-type-traits.wall": 814.9849390983582, "time.clang-tidy.bugprone-use-after-move.wall": 694.5256803035736, "time.clang-tidy.modernize-use-transparent-functors.wall": 670.7836837768555, "time.clang-tidy.bugprone-standalone-empty.wall": 600.5903759002686, "time.clang-tidy.modernize-replace-auto-ptr.wall": 580.4591262340546, "time.clang-tidy.modernize-deprecated-ios-base-aliases.wall": 566.2826192378998, "time.clang-tidy.bugprone-reserved-identifier.wall": 558.5143051147461, "time.clang-tidy.modernize-avoid-c-arrays.wall": 545.1163799762726, "time.clang-tidy.modernize-use-using.wall": 539.6424849033356, "time.clang-tidy.performance-unnecessary-value-param.wall": 471.33301615715027, "time.clang-tidy.readability-non-const-parameter.wall": 440.45192885398865, "time.clang-tidy.cert-dcl58-cpp.wall": 421.88030886650085, "time.clang-tidy.cata-translate-string-literal.wall": 390.8441689014435, "time.clang-tidy.cata-static-initialization-order.wall": 388.08465909957886, "time.clang-tidy.readability-redundant-declaration.wall": 381.59097266197205, "time.clang-tidy.cert-dcl16-c.wall": 362.8544645309448, "time.clang-tidy.readability-container-size-empty.wall": 358.62095737457275, "time.clang-tidy.modernize-use-nullptr.wall": 352.8454167842865, "time.clang-tidy.bugprone-suspicious-string-compare.wall": 350.5792667865753, "time.clang-tidy.misc-misleading-identifier.wall": 346.3509900569916, "time.clang-tidy.misc-definitions-in-headers.wall": 330.5536653995514, "time.clang-tidy.readability-redundant-control-flow.wall": 329.31720495224, "time.clang-tidy.bugprone-infinite-loop.wall": 319.4184067249298, "time.clang-tidy.bugprone-unused-return-value.wall": 306.18062829971313, "time.clang-tidy.performance-move-const-arg.wall": 300.217401266098, "time.clang-tidy.bugprone-assert-side-effect.wall": 298.9822633266449, "time.clang-tidy.bugprone-multiple-statement-macro.wall": 297.0318651199341 } * Fix and re-enable bugprone-multi-level-implicit-pointer-conversion https://clang.llvm.org/extra/clang-tidy/checks/bugprone/multi-level-implicit-pointer-conversion.html Two points of occurence, both are about passing T** to memX() functions. The usage is intended, and correct, so silence. Although perhaps we could add an explicit `reinterpret_cast<void*>` there, but I'm not sure it's valuable. ##[error]/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/cata_bitset.h:182:25: error: multilevel pointer conversion from 'block_t **' (aka 'unsigned long **') to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion,-warnings-as-errors] 182 | memcpy( &ret, &storage_, sizeof( storage_ ) ); |
- Loading branch information