Merge pull request #40041 from jbytheway/enforce_localized_sorting_2

Enforce localized sorting 2
CleverRaven · May 4, 2020 · e635446 · e635446
2 parents f97e96a + 8ca7f94
commit e635446
Show file tree

Hide file tree

Showing 7 changed files with 112 additions and 17 deletions.
diff --git a/src/filesystem.cpp b/src/filesystem.cpp
@@ -315,7 +315,9 @@ std::vector<std::string> find_file_if_bfs( const std::string &root_path,
 
         // Keep files and directories to recurse ordered consistently
         // by sorting from the old end to the new end.
+        // NOLINTNEXTLINE(cata-use-localized-sorting)
         std::sort( std::begin( directories ) + n_dirs,    std::end( directories ) );
+        // NOLINTNEXTLINE(cata-use-localized-sorting)
         std::sort( std::begin( results )     + n_results, std::end( results ) );
     }
 

diff --git a/src/main_menu.cpp b/src/main_menu.cpp
@@ -468,10 +468,11 @@ void main_menu::load_char_templates()
             true ) ) {
         path = native_to_utf8( path );
         path.erase( path.find( ".template" ), std::string::npos );
-        path.erase( 0, path.find_last_of( "\\//" ) + 1 );
+        path.erase( 0, path.find_last_of( "\\/" ) + 1 );
         templates.push_back( path );
     }
-    std::sort( templates.begin(), templates.end(), std::greater<std::string>() );
+    std::sort( templates.begin(), templates.end(), localized_compare );
+    std::reverse( templates.begin(), templates.end() );
 }
 
 bool main_menu::opening_screen()

diff --git a/src/martialarts.cpp b/src/martialarts.cpp
@@ -1465,7 +1465,7 @@ bool ma_style_callback::key( const input_context &ctxt, const input_event &event
             std::transform( ma.weapons.begin(), ma.weapons.end(),
                             std::back_inserter( weapons ), []( const std::string & wid )-> std::string { return item::nname( wid ); } );
             // Sorting alphabetically makes it easier to find a specific weapon
-            std::sort( weapons.begin(), weapons.end() );
+            std::sort( weapons.begin(), weapons.end(), localized_compare );
             // This removes duplicate names (e.g. a real weapon and a replica sharing the same name) from the weapon list.
             auto last = std::unique( weapons.begin(), weapons.end() );
             weapons.erase( last, weapons.end() );

diff --git a/src/requirements.cpp b/src/requirements.cpp
@@ -358,7 +358,7 @@ std::string requirement_data::print_all_objs( const std::string &header,
         []( const T & t ) {
             return t.to_string();
         } );
-        std::sort( alternatives.begin(), alternatives.end() );
+        std::sort( alternatives.begin(), alternatives.end(), localized_compare );
         buffer += join( alternatives, _( " or " ) );
     }
     if( buffer.empty() ) {
@@ -580,7 +580,7 @@ std::vector<std::string> requirement_data::get_folded_list( int width,
             }
             buffer_has.push_back( text + color_tag );
         }
-        std::sort( list_as_string.begin(), list_as_string.end() );
+        std::sort( list_as_string.begin(), list_as_string.end(), localized_compare );
 
         const std::string separator = colorize( _( " OR " ), c_white );
         const std::string unfolded = join( list_as_string, separator );

diff --git a/src/scores_ui.cpp b/src/scores_ui.cpp
@@ -34,7 +34,7 @@ static std::string get_achievements_text( const achievements_tracker &achievemen
                     std::back_inserter( sortable_achievements ),
     [&]( const achievement * ach ) {
         achievement_completion comp = achievements.is_completed( ach->id );
-        return std::make_tuple( comp, ach->description().translated(), ach );
+        return std::make_tuple( comp, ach->name().translated(), ach );
     } );
     std::sort( sortable_achievements.begin(), sortable_achievements.end() );
     for( const sortable_achievement &ach : sortable_achievements ) {

diff --git a/tools/clang-tidy-plugin/UseLocalizedSortingCheck.cpp b/tools/clang-tidy-plugin/UseLocalizedSortingCheck.cpp
@@ -33,29 +33,53 @@ namespace tidy
 namespace cata
 {
 
+inline auto isStringType()
+{
+    return qualType( anyOf( asString( "const std::string" ), asString( "std::string" ) ) );
+}
+
+inline bool IsString( QualType T )
+{
+    const TagDecl *TTag = T.getTypePtr()->getAsTagDecl();
+    if( !TTag ) {
+        return false;
+    }
+    StringRef Name = TTag->getName();
+    return Name == "basic_string";
+}
+
 void UseLocalizedSortingCheck::registerMatchers( MatchFinder *Finder )
 {
     Finder->addMatcher(
         cxxOperatorCallExpr(
             hasArgument(
                 0,
-                expr(
-                    hasType(
-                        qualType(
-                            anyOf( asString( "const std::string" ), asString( "std::string" ) )
-                        ).bind( "arg0Type" )
-                    )
-                ).bind( "arg0Expr" )
+                expr( hasType( isStringType().bind( "arg0Type" ) ) ).bind( "arg0Expr" )
             ),
             hasOverloadedOperatorName( "<" )
-        ).bind( "call" ),
+        ).bind( "opCall" ),
+        this
+    );
+
+    Finder->addMatcher(
+        callExpr(
+            callee( namedDecl( hasAnyName( "std::sort", "std::stable_sort" ) ) ),
+            argumentCountIs( 2 ),
+            hasArgument(
+                0,
+                expr( hasType( qualType( anyOf(
+                        pointerType( pointee( isStringType().bind( "valueType" ) ) ),
+                        hasDeclaration( decl().bind( "iteratorDecl" ) )
+                                         ) ) ) )
+            )
+        ).bind( "sortCall" ),
         this
     );
 }
 
-static void CheckCall( UseLocalizedSortingCheck &Check, const MatchFinder::MatchResult &Result )
+static void CheckOpCall( UseLocalizedSortingCheck &Check, const MatchFinder::MatchResult &Result )
 {
-    const CXXOperatorCallExpr *Call = Result.Nodes.getNodeAs<CXXOperatorCallExpr>( "call" );
+    const CXXOperatorCallExpr *Call = Result.Nodes.getNodeAs<CXXOperatorCallExpr>( "opCall" );
     const QualType *Arg0Type = Result.Nodes.getNodeAs<QualType>( "arg0Type" );
     const Expr *Arg0Expr = Result.Nodes.getNodeAs<Expr>( "arg0Expr" );
     if( !Call || !Arg0Type || !Arg0Expr ) {
@@ -72,9 +96,47 @@ static void CheckCall( UseLocalizedSortingCheck &Check, const MatchFinder::Match
                 "translations.h." ) << *Arg0Type;
 }
 
+static void CheckSortCall( UseLocalizedSortingCheck &Check, const MatchFinder::MatchResult &Result )
+{
+    const CallExpr *Call = Result.Nodes.getNodeAs<CallExpr>( "sortCall" );
+    const QualType *BoundValueType = Result.Nodes.getNodeAs<QualType>( "valueType" );
+    const TagDecl *IteratorDecl = Result.Nodes.getNodeAs<TagDecl>( "iteratorDecl" );
+    if( !Call || ( !BoundValueType && !IteratorDecl ) ) {
+        return;
+    }
+
+    QualType ValueType;
+
+    if( IteratorDecl ) {
+        //Check.diag( Call->getBeginLoc(), "Iterator type %0" ) << IteratorDecl;
+
+        for( const Decl *D : IteratorDecl->decls() ) {
+            if( const TypedefNameDecl *ND = dyn_cast<TypedefNameDecl>( D ) ) {
+                if( ND->getName() == "value_type" ) {
+                    ValueType = ND->getUnderlyingType();
+                    break;
+                }
+            }
+        }
+
+        if( !IsString( ValueType ) ) {
+            return;
+        }
+    }
+
+    if( BoundValueType ) {
+        ValueType = *BoundValueType;
+    }
+
+    Check.diag( Call->getBeginLoc(),
+                "Raw sort of %0.  For UI purposes please use localized_compare from "
+                "translations.h." ) << ValueType;
+}
+
 void UseLocalizedSortingCheck::check( const MatchFinder::MatchResult &Result )
 {
-    CheckCall( *this, Result );
+    CheckOpCall( *this, Result );
+    CheckSortCall( *this, Result );
 }
 
 } // namespace cata

diff --git a/tools/clang-tidy-plugin/test/use-localized-sorting.cpp b/tools/clang-tidy-plugin/test/use-localized-sorting.cpp
@@ -21,8 +21,16 @@ template<class CharT, class Traits, class Alloc>
 bool operator<( const std::basic_string<CharT, Traits, Alloc> &lhs,
                 const std::basic_string<CharT, Traits, Alloc> &rhs ) noexcept;
 
+template<class RandomIt>
+void sort( RandomIt first, RandomIt last );
+
 }
 
+template<class T>
+struct iterator {
+    using value_type = T;
+};
+
 class NonString
 {
 };
@@ -39,3 +47,25 @@ bool f1( const NonString &l, const NonString &r )
 {
     return l < r;
 }
+
+bool sort0( const std::string *start, const std::string *end )
+{
+    std::sort( start, end );
+    // CHECK-MESSAGES: warning: Raw sort of 'const std::string' (aka 'const basic_string<char>').  For UI purposes please use localized_compare from translations.h. [cata-use-localized-sorting]
+}
+
+bool sort1( const NonString *start, const NonString *end )
+{
+    std::sort( start, end );
+}
+
+bool sortit0( iterator<std::string> start, iterator<std::string> end )
+{
+    std::sort( start, end );
+    // CHECK-MESSAGES: warning: Raw sort of 'std::basic_string<char, std::char_traits<char>, std::allocator<char> >'.  For UI purposes please use localized_compare from translations.h. [cata-use-localized-sorting]
+}
+
+bool sortit1( iterator<NonString> start, iterator<NonString> end )
+{
+    std::sort( start, end );
+}