Merge pull request #40291 from jbytheway/enforce_localized_sorting_3

Enforce localized sorting 3
CleverRaven · May 7, 2020 · da1e58c · da1e58c
2 parents 80024b3 + 9625faf
commit da1e58c
Show file tree

Hide file tree

Showing 9 changed files with 115 additions and 23 deletions.
diff --git a/src/cata_variant.h b/src/cata_variant.h
@@ -324,10 +324,10 @@ class cata_variant
     }
         CATA_VARIANT_OPERATOR( == );
         CATA_VARIANT_OPERATOR( != );
-        CATA_VARIANT_OPERATOR( < );
-        CATA_VARIANT_OPERATOR( <= );
-        CATA_VARIANT_OPERATOR( > );
-        CATA_VARIANT_OPERATOR( >= );
+        CATA_VARIANT_OPERATOR( < ); // NOLINT( cata-use-localized-sorting )
+        CATA_VARIANT_OPERATOR( <= ); // NOLINT( cata-use-localized-sorting )
+        CATA_VARIANT_OPERATOR( > ); // NOLINT( cata-use-localized-sorting )
+        CATA_VARIANT_OPERATOR( >= ); // NOLINT( cata-use-localized-sorting )
 #undef CATA_VARIANT_OPERATOR
     private:
         explicit cata_variant( cata_variant_type t, std::string &&v )

diff --git a/src/crafting_gui.cpp b/src/crafting_gui.cpp
@@ -928,6 +928,12 @@ const recipe *select_crafting_recipe( int &batch_size )
 
 std::string peek_related_recipe( const recipe *current, const recipe_subset &available )
 {
+    auto compare_second =
+        []( const std::pair<std::string, std::string> &a,
+    const std::pair<std::string, std::string> &b ) {
+        return localized_compare( a.second, b.second );
+    };
+
     // current recipe components
     std::vector<std::pair<itype_id, std::string>> related_components;
     const requirement_data &req = current->simple_requirements();
@@ -936,6 +942,7 @@ std::string peek_related_recipe( const recipe *current, const recipe_subset &ava
             related_components.push_back( { a.type, item::nname( a.type, 1 ) } );
         }
     }
+    std::sort( related_components.begin(), related_components.end(), compare_second );
     // current recipe result
     std::vector<std::pair<itype_id, std::string>> related_results;
     item tmp = current->create_result();
@@ -947,10 +954,7 @@ std::string peek_related_recipe( const recipe *current, const recipe_subset &ava
             related_results.push_back( { b->result(), b->result_name() } );
         }
     }
-    std::stable_sort( related_results.begin(), related_results.end(),
-    []( const std::pair<std::string, std::string> &a, const std::pair<std::string, std::string> &b ) {
-        return a.second < b.second;
-    } );
+    std::stable_sort( related_results.begin(), related_results.end(), compare_second );
 
     uilist rel_menu;
     int np_last = -1;

diff --git a/src/dump.cpp b/src/dump.cpp
@@ -330,7 +330,7 @@ bool game::dump_stats( const std::string &what, dump_mode mode,
     if( scol >= 0 ) {
         std::sort( rows.begin(), rows.end(), [&scol]( const std::vector<std::string> &lhs,
         const std::vector<std::string> &rhs ) {
-            return lhs[ scol ] < rhs[ scol ];
+            return localized_compare( lhs[ scol ], rhs[ scol ] );
         } );
     }
 

diff --git a/src/flat_set.h b/src/flat_set.h
@@ -13,7 +13,9 @@ struct transparent_less_than {
 
     template<typename T, typename U>
     constexpr auto operator()( T &&lhs, U &&rhs ) const noexcept
+    // NOLINTNEXTLINE(cata-use-localized-sorting)
     -> decltype( std::forward<T>( lhs ) < std::forward<U>( rhs ) ) {
+        // NOLINTNEXTLINE(cata-use-localized-sorting)
         return std::forward<T>( lhs ) < std::forward<U>( rhs );
     }
 };

diff --git a/src/newcharacter.cpp b/src/newcharacter.cpp
@@ -1797,8 +1797,8 @@ tab_direction set_skills( avatar &u, points_left &points )
             std::sort( elem.second.begin(), elem.second.end(),
                        []( const std::pair<std::string, int> &lhs,
             const std::pair<std::string, int> &rhs ) {
-                return lhs.second < rhs.second ||
-                       ( lhs.second == rhs.second && lhs.first < rhs.first );
+                return localized_compare( std::make_pair( lhs.second, lhs.first ),
+                                          std::make_pair( rhs.second, rhs.first ) );
             } );
 
             const std::string rec_temp = enumerate_as_string( elem.second.begin(), elem.second.end(),

diff --git a/src/scores_ui.cpp b/src/scores_ui.cpp
@@ -36,7 +36,7 @@ static std::string get_achievements_text( const achievements_tracker &achievemen
         achievement_completion comp = achievements.is_completed( ach->id );
         return std::make_tuple( comp, ach->name().translated(), ach );
     } );
-    std::sort( sortable_achievements.begin(), sortable_achievements.end() );
+    std::sort( sortable_achievements.begin(), sortable_achievements.end(), localized_compare );
     for( const sortable_achievement &ach : sortable_achievements ) {
         os += achievements.ui_text_for( std::get<const achievement *>( ach ) ) + "\n";
     }

diff --git a/tools/clang-tidy-plugin/UseLocalizedSortingCheck.cpp b/tools/clang-tidy-plugin/UseLocalizedSortingCheck.cpp
@@ -33,19 +33,54 @@ namespace tidy
 namespace cata
 {
 
-inline auto isStringType()
+static bool IsStringish( QualType T );
+
+static bool IsStringish( const TemplateArgument &Arg )
 {
-    return qualType( anyOf( asString( "const std::string" ), asString( "std::string" ) ) );
+    switch( Arg.getKind() ) {
+        case TemplateArgument::Type:
+            if( IsStringish( Arg.getAsType() ) ) {
+                return true;
+            }
+            break;
+        case TemplateArgument::Pack:
+            for( const TemplateArgument &PackArg : Arg.getPackAsArray() ) {
+                if( IsStringish( PackArg ) ) {
+                    return true;
+                }
+            }
+            break;
+        default:
+            break;
+    }
+    return false;
 }
 
-inline bool IsString( QualType T )
+static bool IsStringish( QualType T )
 {
     const TagDecl *TTag = T.getTypePtr()->getAsTagDecl();
     if( !TTag ) {
         return false;
     }
     StringRef Name = TTag->getName();
-    return Name == "basic_string";
+    if( Name == "basic_string" ) {
+        return true;
+    }
+    if( Name == "pair" || Name == "tuple" ) {
+        const ClassTemplateSpecializationDecl *SpecDecl =
+            dyn_cast<ClassTemplateSpecializationDecl>( TTag );
+        if( !SpecDecl ) {
+            fprintf( stderr, "Not a spec: %s\n", TTag->getKindName().str().c_str() );
+            return false;
+        }
+        const TemplateArgumentList &Args = SpecDecl->getTemplateArgs();
+        for( const TemplateArgument &Arg : Args.asArray() ) {
+            if( IsStringish( Arg ) ) {
+                return true;
+            }
+        }
+    }
+    return false;
 }
 
 void UseLocalizedSortingCheck::registerMatchers( MatchFinder *Finder )
@@ -54,9 +89,14 @@ void UseLocalizedSortingCheck::registerMatchers( MatchFinder *Finder )
         cxxOperatorCallExpr(
             hasArgument(
                 0,
-                expr( hasType( isStringType().bind( "arg0Type" ) ) ).bind( "arg0Expr" )
+                expr( hasType( qualType().bind( "arg0Type" ) ) ).bind( "arg0Expr" )
             ),
-            hasOverloadedOperatorName( "<" )
+            anyOf(
+                hasOverloadedOperatorName( "<" ),
+                hasOverloadedOperatorName( ">" ),
+                hasOverloadedOperatorName( "<=" ),
+                hasOverloadedOperatorName( ">=" )
+            )
         ).bind( "opCall" ),
         this
     );
@@ -68,7 +108,7 @@ void UseLocalizedSortingCheck::registerMatchers( MatchFinder *Finder )
             hasArgument(
                 0,
                 expr( hasType( qualType( anyOf(
-                        pointerType( pointee( isStringType().bind( "valueType" ) ) ),
+                        pointerType( pointee( qualType().bind( "valueType" ) ) ),
                         hasDeclaration( decl().bind( "iteratorDecl" ) )
                                          ) ) ) )
             )
@@ -86,6 +126,10 @@ static void CheckOpCall( UseLocalizedSortingCheck &Check, const MatchFinder::Mat
         return;
     }
 
+    if( !IsStringish( *Arg0Type ) ) {
+        return;
+    }
+
     StringRef Arg0Text = getText( Result, Arg0Expr );
     if( Arg0Text.endswith( "id" ) ) {
         return;
@@ -118,16 +162,16 @@ static void CheckSortCall( UseLocalizedSortingCheck &Check, const MatchFinder::M
                 }
             }
         }
-
-        if( !IsString( ValueType ) ) {
-            return;
-        }
     }
 
     if( BoundValueType ) {
         ValueType = *BoundValueType;
     }
 
+    if( !IsStringish( ValueType ) ) {
+        return;
+    }
+
     Check.diag( Call->getBeginLoc(),
                 "Raw sort of %0.  For UI purposes please use localized_compare from "
                 "translations.h." ) << ValueType;

diff --git a/tools/clang-tidy-plugin/UsePointArithmeticCheck.cpp b/tools/clang-tidy-plugin/UsePointArithmeticCheck.cpp
@@ -127,6 +127,7 @@ struct ExpressionComponent {
 
 static bool operator<( const ExpressionComponent &l, const ExpressionComponent &r )
 {
+    // NOLINTNEXTLINE(cata-use-localized-sorting)
     return l.sortKey() < r.sortKey();
 }
 

diff --git a/tools/clang-tidy-plugin/test/use-localized-sorting.cpp b/tools/clang-tidy-plugin/test/use-localized-sorting.cpp
@@ -24,6 +24,18 @@ bool operator<( const std::basic_string<CharT, Traits, Alloc> &lhs,
 template<class RandomIt>
 void sort( RandomIt first, RandomIt last );
 
+template<class T1, class T2>
+struct pair;
+
+template< class T1, class T2 >
+bool operator<( const std::pair<T1, T2> &lhs, const std::pair<T1, T2> &rhs );
+
+template< class... Types >
+class tuple;
+
+template<class... TTypes, class... UTypes>
+bool operator<( const tuple<TTypes...> &lhs,
+                const tuple<UTypes...> &rhs );
 }
 
 template<class T>
@@ -48,6 +60,35 @@ bool f1( const NonString &l, const NonString &r )
     return l < r;
 }
 
+bool f2( const std::pair<int, std::string> &l, const std::pair<int, std::string> &r )
+{
+    return l < r;
+    // CHECK-MESSAGES: warning: Raw comparison of 'const std::pair<int, std::string>' (aka 'const pair<int, basic_string<char> >'). For UI purposes please use localized_compare from translations.h. [cata-use-localized-sorting]
+}
+
+bool f3( const std::pair<int, NonString> &l, const std::pair<int, NonString> &r )
+{
+    return l < r;
+}
+
+bool f4( const std::tuple<int, std::string> &l, const std::tuple<int, std::string> &r )
+{
+    return l < r;
+    // CHECK-MESSAGES: warning: Raw comparison of 'const std::tuple<int, std::string>' (aka 'const tuple<int, basic_string<char> >').  For UI purposes please use localized_compare from translations.h. [cata-use-localized-sorting]
+}
+
+bool f5( const std::tuple<int, NonString> &l, const std::tuple<int, NonString> &r )
+{
+    return l < r;
+}
+
+bool f4( const std::tuple<std::tuple<std::string>> &l,
+         const std::tuple<std::tuple<std::string>> &r )
+{
+    return l < r;
+    // CHECK-MESSAGES: warning: Raw comparison of 'const std::tuple<std::tuple<std::string> >' (aka 'const tuple<tuple<basic_string<char> > >'). For UI purposes please use localized_compare from translations.h. [cata-use-localized-sorting]
+}
+
 bool sort0( const std::string *start, const std::string *end )
 {
     std::sort( start, end );