diff --git a/src/mapdata.cpp b/src/mapdata.cpp index bf699ccfaa760..6fa32be816453 100644 --- a/src/mapdata.cpp +++ b/src/mapdata.cpp @@ -36,7 +36,9 @@ const units::volume DEFAULT_MAX_VOLUME_IN_SQUARE = units::from_liter( 1000 ); generic_factory terrain_data( "terrain" ); generic_factory furniture_data( "furniture" ); +// NOLINTNEXTLINE(cata-static-int_id-constants) ter_id null_ter; +// NOLINTNEXTLINE(cata-static-int_id-constants) furn_id null_furn; ter_t invalid_ter; @@ -757,7 +759,7 @@ void map_data_common_t::set_groups( std::bitset &bits, } } -ter_str_id t_str_null( "t_null" ); +const ter_str_id ter_t_null( "t_null" ); resolved_ter_id t_null, t_hole, // Real nothingness; makes you fall a z-level @@ -883,7 +885,7 @@ resolved_ter_id t_null, void set_ter_ids() { - t_null = null_ter = t_str_null; + t_null = null_ter = ter_t_null; t_hole = ter_id( "t_hole" ); t_dirt = ter_id( "t_dirt" ); t_sand = ter_id( "t_sand" ); @@ -1192,7 +1194,7 @@ void reset_furn_ter() furniture_data.reset(); } -static furn_str_id f_str_null( "f_null" ); +static const furn_str_id furn_t_null( "f_null" ); resolved_furn_id f_null, f_clear, f_hay, @@ -1242,7 +1244,7 @@ resolved_furn_id f_null, f_clear, void set_furn_ids() { - f_null = null_furn = f_str_null; + f_null = null_furn = furn_t_null; f_clear = furn_id( "f_clear" ); f_hay = furn_id( "f_hay" ); f_rubble = furn_id( "f_rubble" ); @@ -1805,7 +1807,7 @@ void activity_data_ter::load( const JsonObject &jo ) void activity_data_furn::load( const JsonObject &jo ) { - optional( jo, was_loaded, "result", result_, f_str_null ); + optional( jo, was_loaded, "result", result_, furn_t_null ); activity_data_common::load( jo ); valid_ = true; } diff --git a/src/mapdata.h b/src/mapdata.h index 576e7885b0961..c23b67c5d2085 100644 --- a/src/mapdata.h +++ b/src/mapdata.h @@ -690,8 +690,8 @@ provided for terrains added by mods. A string equivalent is always present, i.e. t_basalt "t_basalt" */ -extern ter_str_id t_str_null; -// NOLINTNEXTLINE(cata-static-int_id-constants) +extern const ter_str_id ter_t_null; +// NOLINTNEXTLINE(cata-static-resolved_id-constants) extern resolved_ter_id t_null, t_hole, // Real nothingness; makes you fall a z-level // Ground @@ -817,7 +817,7 @@ runtime index: furn_id furn_id refers to a position in the furnlist[] where the furn_t struct is stored. See note about ter_id above. */ -// NOLINTNEXTLINE(cata-static-int_id-constants) +// NOLINTNEXTLINE(cata-static-resolved_id-constants) extern resolved_furn_id f_null, f_clear, f_hay, f_cattails, f_lotus, f_lilypad, f_rubble, f_rubble_rock, f_wreckage, f_ash, diff --git a/src/mapgen.cpp b/src/mapgen.cpp index 7914b80880e73..5c1bc4c79db53 100644 --- a/src/mapgen.cpp +++ b/src/mapgen.cpp @@ -825,7 +825,7 @@ mapgen_function_json::mapgen_function_json( const JsonObject &jsobj, dbl_or_var w, const std::string &context, const point &grid_offset, const point &grid_total ) : mapgen_function( std::move( w ) ) , mapgen_function_json_base( jsobj, context ) - , fill_ter( t_str_null ) + , fill_ter( ter_t_null ) , rotation( 0 ) , fallback_predecessor_mapgen_( oter_str_id::NULL_ID() ) { @@ -4352,7 +4352,7 @@ bool mapgen_function_json::setup_internal( const JsonObject &jo ) jo.read( "fallback_predecessor_mapgen", fallback_predecessor_mapgen_ ); - return fill_ter != t_str_null || predecessor_mapgen != oter_str_id::NULL_ID() || + return fill_ter != ter_t_null || predecessor_mapgen != oter_str_id::NULL_ID() || fallback_predecessor_mapgen_ != oter_str_id::NULL_ID(); } @@ -5120,7 +5120,7 @@ static bool apply_mapgen_in_phases( void mapgen_function_json::generate( mapgendata &md ) { map *const m = &md.m; - if( fill_ter != t_str_null ) { + if( fill_ter != ter_t_null ) { m->draw_fill_background( fill_ter ); } const oter_t &ter = *md.terrain_type(); @@ -7653,7 +7653,7 @@ bool update_mapgen_function_json::setup_update( const JsonObject &jo ) bool update_mapgen_function_json::setup_internal( const JsonObject &/*jo*/ ) { - fill_ter = t_str_null; + fill_ter = ter_t_null; /* update_mapgen doesn't care about fill_ter or rows */ return true; } diff --git a/src/sounds.cpp b/src/sounds.cpp index 5fa68d350a90c..9b2e3ac140572 100644 --- a/src/sounds.cpp +++ b/src/sounds.cpp @@ -1696,7 +1696,7 @@ void sfx::do_footstep() if( std::chrono::duration_cast ( sfx_time ).count() > 400 ) { const Character &player_character = get_player_character(); int heard_volume = sfx::get_heard_volume( player_character.pos() ); - const auto terrain = get_map().ter( player_character.pos() )->id; + const ter_str_id terrain = get_map().ter( player_character.pos() )->id; static const std::set grass = { ter_t_grass, ter_t_shrub, diff --git a/src/timed_event.cpp b/src/timed_event.cpp index 31fc8f1c3996a..d31feb661b22e 100644 --- a/src/timed_event.cpp +++ b/src/timed_event.cpp @@ -196,7 +196,8 @@ void timed_event::actualize() case timed_event_type::TEMPLE_FLOOD: { bool flooded = false; - cata::mdarray flood_buf; + std::vector> flood_buf( MAPSIZE_X, + std::vector( MAPSIZE_Y, t_null ) ); for( const tripoint &p : here.points_on_zlevel() ) { flood_buf[p.x][p.y] = here.ter( p ); } diff --git a/tools/clang-tidy-plugin/CMakeLists.txt b/tools/clang-tidy-plugin/CMakeLists.txt index 22a1fe639b1a1..45f1056b3d7a2 100644 --- a/tools/clang-tidy-plugin/CMakeLists.txt +++ b/tools/clang-tidy-plugin/CMakeLists.txt @@ -25,6 +25,7 @@ set(CataAnalyzerSrc StaticDeclarationsCheck.cpp StaticInitializationOrderCheck.cpp StaticIntIdConstantsCheck.cpp + StaticResolvedIdConstantsCheck.cpp StaticStringIdConstantsCheck.cpp StringLiteralIterator.cpp TestFilenameCheck.cpp diff --git a/tools/clang-tidy-plugin/CataTidyModule.cpp b/tools/clang-tidy-plugin/CataTidyModule.cpp index 04d8b5596c929..6ba67811aaa96 100644 --- a/tools/clang-tidy-plugin/CataTidyModule.cpp +++ b/tools/clang-tidy-plugin/CataTidyModule.cpp @@ -22,6 +22,7 @@ #include "StaticDeclarationsCheck.h" #include "StaticInitializationOrderCheck.h" #include "StaticIntIdConstantsCheck.h" +#include "StaticResolvedIdConstantsCheck.h" #include "StaticStringIdConstantsCheck.h" #include "TestFilenameCheck.h" #include "TestsMustRestoreGlobalStateCheck.h" @@ -88,6 +89,8 @@ class CataModule : public ClangTidyModule CheckFactories.registerCheck( "cata-static-declarations" ); CheckFactories.registerCheck( "cata-static-int_id-constants" ); + CheckFactories.registerCheck( + "cata-static-resolved_id-constants" ); CheckFactories.registerCheck( "cata-static-string_id-constants" ); CheckFactories.registerCheck( "cata-test-filename" ); diff --git a/tools/clang-tidy-plugin/StaticResolvedIdConstantsCheck.cpp b/tools/clang-tidy-plugin/StaticResolvedIdConstantsCheck.cpp new file mode 100644 index 0000000000000..7e695f71a4feb --- /dev/null +++ b/tools/clang-tidy-plugin/StaticResolvedIdConstantsCheck.cpp @@ -0,0 +1,75 @@ +#include "StaticResolvedIdConstantsCheck.h" +#include "Utils.h" +#include + +using namespace clang::ast_matchers; + +namespace clang::tidy::cata +{ + +void StaticResolvedIdConstantsCheck::registerMatchers( MatchFinder *Finder ) +{ + Finder->addMatcher( + varDecl( + hasType( + namedDecl( + anyOf( + hasName( "resolved_id" ), + typedefNameDecl( + hasType( + hasCanonicalType( + hasDeclaration( namedDecl( hasName( "resolved_id" ) ) ) + ) + ) + ) + ) + ).bind( "typeDecl" ) + ) + ).bind( "varDecl" ), + this + ); +} + +static void CheckConstructor( StaticResolvedIdConstantsCheck &Check, + const MatchFinder::MatchResult &Result ) +{ + const VarDecl *ResolvedIdVarDecl = Result.Nodes.getNodeAs( "varDecl" ); + const TypeDecl *ResolvedIdTypeDecl = Result.Nodes.getNodeAs( "typeDecl" ); + if( !ResolvedIdVarDecl || !ResolvedIdTypeDecl ) { + return; + } + + const VarDecl *PreviousDecl = dyn_cast_or_null( ResolvedIdVarDecl->getPreviousDecl() ); + + if( PreviousDecl ) { + // Only complain about each variable once + return; + } + + std::string Adjective; + + if( ResolvedIdVarDecl->hasGlobalStorage() ) { + Adjective = "Global"; + } + + if( ResolvedIdVarDecl->isStaticDataMember() || ResolvedIdVarDecl->isStaticLocal() ) { + Adjective = "Static"; + } + + if( Adjective.empty() ) { + return; + } + + Check.diag( + ResolvedIdVarDecl->getBeginLoc(), + "%2 declaration of %0 is dangerous because %1 is a specialization of resolved_id and it " + "will not update automatically when game data changes. Consider switching to a string_id." + ) << ResolvedIdVarDecl << ResolvedIdTypeDecl << Adjective; +} + +void StaticResolvedIdConstantsCheck::check( const MatchFinder::MatchResult &Result ) +{ + CheckConstructor( *this, Result ); +} + +} // namespace clang::tidy::cata diff --git a/tools/clang-tidy-plugin/StaticResolvedIdConstantsCheck.h b/tools/clang-tidy-plugin/StaticResolvedIdConstantsCheck.h new file mode 100644 index 0000000000000..39450b71de77e --- /dev/null +++ b/tools/clang-tidy-plugin/StaticResolvedIdConstantsCheck.h @@ -0,0 +1,33 @@ +#ifndef CATA_TOOLS_CLANG_TIDY_PLUGIN_STATICRESOLVEDIDCONSTANTSCHECK_H +#define CATA_TOOLS_CLANG_TIDY_PLUGIN_STATICRESOLVEDIDCONSTANTSCHECK_H + +#include +#include + +#include +#include + +namespace clang +{ + +namespace tidy +{ +class ClangTidyContext; + +namespace cata +{ + +class StaticResolvedIdConstantsCheck : public ClangTidyCheck +{ + public: + StaticResolvedIdConstantsCheck( StringRef Name, ClangTidyContext *Context ) + : ClangTidyCheck( Name, Context ) {} + void registerMatchers( ast_matchers::MatchFinder *Finder ) override; + void check( const ast_matchers::MatchFinder::MatchResult &Result ) override; +}; + +} // namespace cata +} // namespace tidy +} // namespace clang + +#endif // CATA_TOOLS_CLANG_TIDY_PLUGIN_STATICRESOLVEDIDCONSTANTSCHECK_H diff --git a/tools/clang-tidy-plugin/test/static-resolved_id-constants.cpp b/tools/clang-tidy-plugin/test/static-resolved_id-constants.cpp new file mode 100644 index 0000000000000..1433257b4f226 --- /dev/null +++ b/tools/clang-tidy-plugin/test/static-resolved_id-constants.cpp @@ -0,0 +1,41 @@ +// RUN: %check_clang_tidy -allow-stdinc %s cata-static-resolved_id-constants %t -- --load=%cata_plugin -- -isystem %cata_include + +#include "type_id.h" + +extern resolved_furn_id f_hay; +// CHECK-MESSAGES: warning: Global declaration of 'f_hay' is dangerous because 'resolved_furn_id' is a specialization of resolved_id and it will not update automatically when game data changes. Consider switching to a string_id. [cata-static-resolved_id-constants] + +// No warning for second decl os same variable. +resolved_furn_id f_hay; + +static resolved_id f_ball_mach; +// CHECK-MESSAGES: warning: Global declaration of 'f_ball_mach' is dangerous because 'resolved_id' is a specialization of resolved_id and it will not update automatically when game data changes. Consider switching to a string_id. [cata-static-resolved_id-constants] + +namespace foo +{ + +const resolved_furn_id f_dahlia; +// CHECK-MESSAGES: warning: Global declaration of 'f_dahlia' is dangerous because 'resolved_furn_id' is a specialization of resolved_id and it will not update automatically when game data changes. Consider switching to a string_id. [cata-static-resolved_id-constants] + +} // namespace foo + +class A +{ + static resolved_furn_id f_bluebell; + // CHECK-MESSAGES: warning: Static declaration of 'f_bluebell' is dangerous because 'resolved_furn_id' is a specialization of resolved_id and it will not update automatically when game data changes. Consider switching to a string_id. [cata-static-resolved_id-constants] + + // No warning for regular class data members + resolved_furn_id my_furn; +}; + +void f() +{ + static resolved_furn_id f_floor_canvas; + // CHECK-MESSAGES: warning: Static declaration of 'f_floor_canvas' is dangerous because 'resolved_furn_id' is a specialization of resolved_id and it will not update automatically when game data changes. Consider switching to a string_id. [cata-static-resolved_id-constants] + + // No warning for regular local variables + resolved_furn_id f; + + // No warning for other types + static int i; +}