Add custom clang-tidy check for fields that could be replaced by poin…

…ts (#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

@@ -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 )

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 )
 {
 }

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:

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;

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;

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;

tools/clang-tidy-plugin/CMakeLists.txt

@@ -8,6 +8,7 @@ add_library(
     CataTidyModule.cpp
     NoLongCheck.cpp
     PointInitializationCheck.cpp
+    XYCheck.cpp
     )
 
 target_include_directories(

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<NoLongCheck>( "cata-no-long" );
             CheckFactories.registerCheck<PointInitializationCheck>( "cata-point-initialization" );
+            CheckFactories.registerCheck<XYCheck>( "cata-xy" );
         }
 };
 

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<FieldDecl>( "xfield" );
+    const FieldDecl *YVar = Result.Nodes.getNodeAs<FieldDecl>( "yfield" );
+    const CXXRecordDecl *Record = Result.Nodes.getNodeAs<CXXRecordDecl>( "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

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 <clang/ASTMatchers/ASTMatchFinder.h>
+#include <llvm/ADT/StringRef.h>
+
+#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

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<int>
+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;
+};