From a7a3646148e76c70c74bce9765a4e1dfcde2433d Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Wed, 29 Apr 2020 21:43:57 -0400 Subject: [PATCH 1/5] Expand sorting check to std::sort calls Look for calls which sort strings in a non-localized manner. --- .../UseLocalizedSortingCheck.cpp | 84 ++++++++++++++++--- .../test/use-localized-sorting.cpp | 30 +++++++ 2 files changed, 103 insertions(+), 11 deletions(-) diff --git a/tools/clang-tidy-plugin/UseLocalizedSortingCheck.cpp b/tools/clang-tidy-plugin/UseLocalizedSortingCheck.cpp index e579cc425338e..f7ea445e49382 100644 --- 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( "call" ); + const CXXOperatorCallExpr *Call = Result.Nodes.getNodeAs( "opCall" ); const QualType *Arg0Type = Result.Nodes.getNodeAs( "arg0Type" ); const Expr *Arg0Expr = Result.Nodes.getNodeAs( "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( "sortCall" ); + const QualType *BoundValueType = Result.Nodes.getNodeAs( "valueType" ); + const TagDecl *IteratorDecl = Result.Nodes.getNodeAs( "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( 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 index bdc23d552bd0a..1ecea6e77c218 100644 --- 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 bool operator<( const std::basic_string &lhs, const std::basic_string &rhs ) noexcept; +template +void sort( RandomIt first, RandomIt last ); + } +template +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'). 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 start, iterator end ) +{ + std::sort( start, end ); + // CHECK-MESSAGES: warning: Raw sort of 'std::basic_string, std::allocator >'. For UI purposes please use localized_compare from translations.h. [cata-use-localized-sorting] +} + +bool sortit1( iterator start, iterator end ) +{ + std::sort( start, end ); +} From ce441bf17a58f54bff7ae6e0f1ae7af7de32b491 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Thu, 30 Apr 2020 07:32:38 -0400 Subject: [PATCH 2/5] Sort achievements by name, not description --- src/scores_ui.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/scores_ui.cpp b/src/scores_ui.cpp index 59f837fe1642c..3261f563a1f83 100644 --- 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 ) { From 76cf8b6289d294b26b077cfcb58ed6bbf03a9748 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Thu, 30 Apr 2020 07:47:31 -0400 Subject: [PATCH 3/5] Remove unnecessary '/' character --- src/main_menu.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main_menu.cpp b/src/main_menu.cpp index ce2c7db86fde6..6035f663ec388 100644 --- a/src/main_menu.cpp +++ b/src/main_menu.cpp @@ -468,7 +468,7 @@ 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() ); From 4402b90f22bd03a052da87cd2a870d9bbc1ee8bb Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Thu, 30 Apr 2020 07:48:18 -0400 Subject: [PATCH 4/5] Suppress warning on sorting directory names --- src/filesystem.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/filesystem.cpp b/src/filesystem.cpp index 4a450dca67769..f23472836f9f8 100644 --- a/src/filesystem.cpp +++ b/src/filesystem.cpp @@ -315,7 +315,9 @@ std::vector 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 ) ); } From 8ca7f947e55b96c0e0e4da3449fbd6ba51228c51 Mon Sep 17 00:00:00 2001 From: John Bytheway Date: Thu, 30 Apr 2020 07:48:44 -0400 Subject: [PATCH 5/5] Convert some std::sort calls to localized_compare These are the ones that were caught by the recently improved check. --- src/main_menu.cpp | 3 ++- src/martialarts.cpp | 2 +- src/requirements.cpp | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/main_menu.cpp b/src/main_menu.cpp index 6035f663ec388..e939ad760ddc0 100644 --- a/src/main_menu.cpp +++ b/src/main_menu.cpp @@ -471,7 +471,8 @@ void main_menu::load_char_templates() path.erase( 0, path.find_last_of( "\\/" ) + 1 ); templates.push_back( path ); } - std::sort( templates.begin(), templates.end(), std::greater() ); + 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 index 5e4fa16be8658..a881ae6f1c2d2 100644 --- 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 index 7949f26cdbf4e..f896df9aeda33 100644 --- 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 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 );