Skip to content

Commit

Permalink
Add clang-tidy checks for global resolved_ids and fix some assorted…
Browse files Browse the repository at this point in the history
… clang-tidy errors.
  • Loading branch information
prharvey committed Dec 27, 2023
1 parent 462ee91 commit 9a829b3
Show file tree
Hide file tree
Showing 10 changed files with 170 additions and 14 deletions.
12 changes: 7 additions & 5 deletions src/mapdata.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ const units::volume DEFAULT_MAX_VOLUME_IN_SQUARE = units::from_liter( 1000 );
generic_factory<ter_t> terrain_data( "terrain" );
generic_factory<furn_t> 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;
Expand Down Expand Up @@ -757,7 +759,7 @@ void map_data_common_t::set_groups( std::bitset<NUM_TERCONN> &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
Expand Down Expand Up @@ -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" );
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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" );
Expand Down Expand Up @@ -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;
}
Expand Down
6 changes: 3 additions & 3 deletions src/mapdata.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
8 changes: 4 additions & 4 deletions src/mapgen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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() )
{
Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion src/sounds.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1696,7 +1696,7 @@ void sfx::do_footstep()
if( std::chrono::duration_cast<std::chrono::milliseconds> ( 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<ter_str_id> grass = {
ter_t_grass,
ter_t_shrub,
Expand Down
3 changes: 2 additions & 1 deletion src/timed_event.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,8 @@ void timed_event::actualize()
case timed_event_type::TEMPLE_FLOOD: {
bool flooded = false;

cata::mdarray<resolved_ter_id, point_bub_ms> flood_buf;
std::vector<std::vector<resolved_ter_id>> flood_buf( MAPSIZE_X,
std::vector<resolved_ter_id>( MAPSIZE_Y, t_null ) );
for( const tripoint &p : here.points_on_zlevel() ) {
flood_buf[p.x][p.y] = here.ter( p );
}
Expand Down
1 change: 1 addition & 0 deletions tools/clang-tidy-plugin/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ set(CataAnalyzerSrc
StaticDeclarationsCheck.cpp
StaticInitializationOrderCheck.cpp
StaticIntIdConstantsCheck.cpp
StaticResolvedIdConstantsCheck.cpp
StaticStringIdConstantsCheck.cpp
StringLiteralIterator.cpp
TestFilenameCheck.cpp
Expand Down
3 changes: 3 additions & 0 deletions tools/clang-tidy-plugin/CataTidyModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -88,6 +89,8 @@ class CataModule : public ClangTidyModule
CheckFactories.registerCheck<StaticDeclarationsCheck>( "cata-static-declarations" );
CheckFactories.registerCheck<StaticIntIdConstantsCheck>(
"cata-static-int_id-constants" );
CheckFactories.registerCheck<StaticResolvedIdConstantsCheck>(
"cata-static-resolved_id-constants" );
CheckFactories.registerCheck<StaticStringIdConstantsCheck>(
"cata-static-string_id-constants" );
CheckFactories.registerCheck<TestFilenameCheck>( "cata-test-filename" );
Expand Down
75 changes: 75 additions & 0 deletions tools/clang-tidy-plugin/StaticResolvedIdConstantsCheck.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
#include "StaticResolvedIdConstantsCheck.h"
#include "Utils.h"
#include <unordered_map>

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>( "varDecl" );
const TypeDecl *ResolvedIdTypeDecl = Result.Nodes.getNodeAs<TypeDecl>( "typeDecl" );
if( !ResolvedIdVarDecl || !ResolvedIdTypeDecl ) {
return;
}

const VarDecl *PreviousDecl = dyn_cast_or_null<VarDecl>( 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
33 changes: 33 additions & 0 deletions tools/clang-tidy-plugin/StaticResolvedIdConstantsCheck.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#ifndef CATA_TOOLS_CLANG_TIDY_PLUGIN_STATICRESOLVEDIDCONSTANTSCHECK_H
#define CATA_TOOLS_CLANG_TIDY_PLUGIN_STATICRESOLVEDIDCONSTANTSCHECK_H

#include <clang/ASTMatchers/ASTMatchFinder.h>
#include <llvm/ADT/StringRef.h>

#include <clang-tidy/ClangTidy.h>
#include <clang-tidy/ClangTidyCheck.h>

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
41 changes: 41 additions & 0 deletions tools/clang-tidy-plugin/test/static-resolved_id-constants.cpp
Original file line number Diff line number Diff line change
@@ -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<furn_t> f_ball_mach;
// CHECK-MESSAGES: warning: Global declaration of 'f_ball_mach' is dangerous because 'resolved_id<furn_t>' 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;
}

0 comments on commit 9a829b3

Please sign in to comment.