From 6e4bd900968b19cc07dd55aaa6f7f1a24e6dc0c7 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Fri, 2 Aug 2019 12:52:56 -0400 Subject: [PATCH] Add custom clang-tidy check for fields that could be replaced by points (#32852) * Add clang-tidy check for structs with x,y members * Suppress cata-xy warnings in various places * Simplify boolean expression * Remove redundant point initialization --- src/map.cpp | 6 +- src/mapgen.cpp | 3 +- src/messages.cpp | 1 + src/point.h | 2 + src/string_input_popup.h | 2 +- src/ui.h | 2 +- tools/clang-tidy-plugin/CMakeLists.txt | 1 + tools/clang-tidy-plugin/CataTidyModule.cpp | 2 + tools/clang-tidy-plugin/XYCheck.cpp | 83 ++++++++++++++++++++++ tools/clang-tidy-plugin/XYCheck.h | 33 +++++++++ tools/clang-tidy-plugin/test/xy.cpp | 57 +++++++++++++++ 11 files changed, 183 insertions(+), 9 deletions(-) create mode 100644 tools/clang-tidy-plugin/XYCheck.cpp create mode 100644 tools/clang-tidy-plugin/XYCheck.h create mode 100644 tools/clang-tidy-plugin/test/xy.cpp diff --git a/src/map.cpp b/src/map.cpp index bbb882823d469..dd580bd753d35 100644 --- a/src/map.cpp +++ b/src/map.cpp @@ -1310,11 +1310,7 @@ bool map::can_move_furniture( const tripoint &pos, player *p ) adjusted_str = mons->mech_str_addition(); } } - if( adjusted_str < required_str ) { - return false; - } - - return true; + return adjusted_str >= required_str; } std::string map::furnname( const tripoint &p ) diff --git a/src/mapgen.cpp b/src/mapgen.cpp index 32d0e255b1161..3ab457ee9c713 100644 --- a/src/mapgen.cpp +++ b/src/mapgen.cpp @@ -412,7 +412,7 @@ void load_mapgen( JsonObject &jo ) if( jo.has_array( "om_terrain" ) ) { JsonArray ja = jo.get_array( "om_terrain" ); if( ja.test_array() ) { - point offset = point_zero; + point offset; while( ja.has_more() ) { JsonArray row_items = ja.next_array(); while( row_items.has_more() ) { @@ -507,7 +507,6 @@ mapgen_function_json_base::mapgen_function_json_base( const std::string &s ) , do_format( false ) , is_ready( false ) , mapgensize( SEEX * 2, SEEY * 2 ) - , m_offset( point_zero ) , objects( m_offset, mapgensize ) { } diff --git a/src/messages.cpp b/src/messages.cpp index 02bcf95715737..8eab8b9410460 100644 --- a/src/messages.cpp +++ b/src/messages.cpp @@ -398,6 +398,7 @@ static bool msg_type_from_name( game_message_type &type, const std::string &name namespace Messages { +// NOLINTNEXTLINE(cata-xy) class dialog { public: diff --git a/src/point.h b/src/point.h index 879c040f47d03..90d746b36f74f 100644 --- a/src/point.h +++ b/src/point.h @@ -11,6 +11,7 @@ class JsonOut; class JsonIn; +// NOLINTNEXTLINE(cata-xy) struct point { int x = 0; int y = 0; @@ -111,6 +112,7 @@ inline point abs( const point &p ) return point( abs( p.x ), abs( p.y ) ); } +// NOLINTNEXTLINE(cata-xy) struct tripoint { int x = 0; int y = 0; diff --git a/src/string_input_popup.h b/src/string_input_popup.h index aae80363c702a..f8124aa849fa8 100644 --- a/src/string_input_popup.h +++ b/src/string_input_popup.h @@ -41,7 +41,7 @@ class utf8_wrapper; * ignored and the returned string is never longer than this. * @param only_digits Whether to only allow digits in the string. */ -class string_input_popup +class string_input_popup // NOLINT(cata-xy) { private: std::string _title; diff --git a/src/ui.h b/src/ui.h index 0299e34a9d724..2f5dbe0bc615f 100644 --- a/src/ui.h +++ b/src/ui.h @@ -89,7 +89,7 @@ struct uilist_entry { /** * Virtual base class for windowed ui stuff (like uilist) */ -class ui_container +class ui_container // NOLINT(cata-xy) { public: virtual ~ui_container() = default; diff --git a/tools/clang-tidy-plugin/CMakeLists.txt b/tools/clang-tidy-plugin/CMakeLists.txt index a7561aed619a5..3167cb14b1797 100644 --- a/tools/clang-tidy-plugin/CMakeLists.txt +++ b/tools/clang-tidy-plugin/CMakeLists.txt @@ -8,6 +8,7 @@ add_library( CataTidyModule.cpp NoLongCheck.cpp PointInitializationCheck.cpp + XYCheck.cpp ) target_include_directories( diff --git a/tools/clang-tidy-plugin/CataTidyModule.cpp b/tools/clang-tidy-plugin/CataTidyModule.cpp index 64ed1756322ca..f6696b04a4ae1 100644 --- a/tools/clang-tidy-plugin/CataTidyModule.cpp +++ b/tools/clang-tidy-plugin/CataTidyModule.cpp @@ -4,6 +4,7 @@ #include "ClangTidyModuleRegistry.h" #include "NoLongCheck.h" #include "PointInitializationCheck.h" +#include "XYCheck.h" namespace clang { @@ -18,6 +19,7 @@ class CataModule : public ClangTidyModule void addCheckFactories( ClangTidyCheckFactories &CheckFactories ) override { CheckFactories.registerCheck( "cata-no-long" ); CheckFactories.registerCheck( "cata-point-initialization" ); + CheckFactories.registerCheck( "cata-xy" ); } }; diff --git a/tools/clang-tidy-plugin/XYCheck.cpp b/tools/clang-tidy-plugin/XYCheck.cpp new file mode 100644 index 0000000000000..8b3ac0a301de2 --- /dev/null +++ b/tools/clang-tidy-plugin/XYCheck.cpp @@ -0,0 +1,83 @@ +#include "XYCheck.h" + +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Frontend/CompilerInstance.h" + +using namespace clang::ast_matchers; + +namespace clang +{ +namespace tidy +{ +namespace cata +{ + +void XYCheck::registerMatchers( MatchFinder *Finder ) +{ + Finder->addMatcher( + fieldDecl( + hasType( asString( "int" ) ), + matchesName( "x$" ), + hasParent( + cxxRecordDecl( + forEachDescendant( fieldDecl( matchesName( "y$" ) ).bind( "yfield" ) ) + ).bind( "record" ) + ) + ).bind( "xfield" ), + this + ); +} + +static void CheckField( XYCheck &Check, const MatchFinder::MatchResult &Result ) +{ + const FieldDecl *XVar = Result.Nodes.getNodeAs( "xfield" ); + const FieldDecl *YVar = Result.Nodes.getNodeAs( "yfield" ); + const CXXRecordDecl *Record = Result.Nodes.getNodeAs( "record" ); + if( !XVar || !YVar || !Record ) { + return; + } + llvm::StringRef XPrefix = XVar->getName().drop_back(); + llvm::StringRef YPrefix = YVar->getName().drop_back(); + if( XPrefix != YPrefix ) { + return; + } + + const FieldDecl *ZVar = nullptr; + for( FieldDecl *Field : Record->fields() ) { + StringRef Name = Field->getName(); + if( Name.endswith( "z" ) && Name.drop_back() == XPrefix ) { + ZVar = Field; + break; + } + } + TemplateSpecializationKind tsk = Record->getTemplateSpecializationKind(); + if( tsk != TSK_Undeclared ) { + // Avoid duplicate warnings for specializations + return; + } + if( ZVar ) { + Check.diag( + Record->getLocation(), + "%0 defines fields %1, %2, and %3. Consider combining into a single tripoint " + "field." ) << Record << XVar << YVar << ZVar; + } else { + Check.diag( + Record->getLocation(), + "%0 defines fields %1 and %2. Consider combining into a single point " + "field." ) << Record << XVar << YVar; + } + Check.diag( XVar->getLocation(), "declaration of %0", DiagnosticIDs::Note ) << XVar; + Check.diag( YVar->getLocation(), "declaration of %0", DiagnosticIDs::Note ) << YVar; + if( ZVar ) { + Check.diag( ZVar->getLocation(), "declaration of %0", DiagnosticIDs::Note ) << ZVar; + } +} + +void XYCheck::check( const MatchFinder::MatchResult &Result ) +{ + CheckField( *this, Result ); +} + +} // namespace cata +} // namespace tidy +} // namespace clang diff --git a/tools/clang-tidy-plugin/XYCheck.h b/tools/clang-tidy-plugin/XYCheck.h new file mode 100644 index 0000000000000..f871fe5e9c827 --- /dev/null +++ b/tools/clang-tidy-plugin/XYCheck.h @@ -0,0 +1,33 @@ +#ifndef CATA_TOOLS_CLANG_TIDY_XYCHECK_H +#define CATA_TOOLS_CLANG_TIDY_XYCHECK_H + +#include +#include + +#include "ClangTidy.h" + +namespace clang +{ +class CompilerInstance; + +namespace tidy +{ +class ClangTidyContext; + +namespace cata +{ + +class XYCheck : public ClangTidyCheck +{ + public: + XYCheck( 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_XYCHECK_H diff --git a/tools/clang-tidy-plugin/test/xy.cpp b/tools/clang-tidy-plugin/test/xy.cpp new file mode 100644 index 0000000000000..8b089ecf7186f --- /dev/null +++ b/tools/clang-tidy-plugin/test/xy.cpp @@ -0,0 +1,57 @@ +// RUN: %check_clang_tidy %s cata-xy %t -- -plugins=%cata_plugin -- + +struct A0 { + // CHECK-MESSAGES: warning: 'A0' defines fields 'x' and 'y'. Consider combining into a single point field. [cata-xy] + int x; + int y; +}; + +struct A1 { + // CHECK-MESSAGES: warning: 'A1' defines fields 'foox' and 'fooy'. Consider combining into a single point field. [cata-xy] + int foox; + int fooy; +}; + +struct A2 { + int foox; + int bary; +}; + +struct A3 { + // CHECK-MESSAGES: warning: 'A3' defines fields 'foox' and 'fooy'. Consider combining into a single point field. [cata-xy] + int foox; + int bary; + int fooy; +}; + +struct A4 { + // CHECK-MESSAGES: warning: 'A4' defines fields 'barx' and 'bary'. Consider combining into a single point field. [cata-xy] + // CHECK-MESSAGES: warning: 'A4' defines fields 'foox' and 'fooy'. Consider combining into a single point field. [cata-xy] + int foox; + int fooy; + int barx; + int bary; +}; + +struct B { + // CHECK-MESSAGES: warning: 'B' defines fields 'x', 'y', and 'z'. Consider combining into a single tripoint field. [cata-xy] + int x; + int y; + int z; +}; + +template +struct C { + // CHECK-MESSAGES: warning: 'C' defines fields 'x' and 'y'. Consider combining into a single point field. [cata-xy] + int x; + int y; +}; + +C<0> c0; +C<1> c1; + +struct D { + // Verify that there are no warnings for non-int types + float x; + float y; +};