Replace another StrPrintf usage, add ReplaceIllegalFileNameChars unit tests

common/io/eagle/eagle_parser.cpp

@@ -1476,7 +1476,7 @@ EELEMENT::EELEMENT( wxXmlNode* aElement, IO_BASE* aIo ) :
     library = parseRequiredAttribute<wxString>( aElement, "library" );
     value   = parseRequiredAttribute<wxString>( aElement, "value" );
     std::string p = parseRequiredAttribute<std::string>( aElement, "package" );
-    ReplaceIllegalFileNameChars( &p, '_' );
+    ReplaceIllegalFileNameChars( p, '_' );
     package = wxString::FromUTF8( p.c_str() );
 
     x       = parseRequiredAttribute<ECOORD>( aElement, "x" );
@@ -1732,7 +1732,7 @@ EDEVICE::EDEVICE( wxXmlNode* aDevice, IO_BASE* aIo ) :
     if( pack )
     {
         std::string p( pack->c_str() );
-        ReplaceIllegalFileNameChars( &p, '_' );
+        ReplaceIllegalFileNameChars( p, '_' );
         package.Set( wxString::FromUTF8( p.c_str() ) );
     }
 

common/string_utils.cpp

@@ -48,7 +48,7 @@
  * platforms.  This is the list of illegal file name characters for Windows which includes
  * the illegal file name characters for Linux and OSX.
  */
-static const char illegalFileNameChars[] = "\\/:\"<>|*?";
+static constexpr std::string_view illegalFileNameChars = "\\/:\"<>|*?";
 
 static const wxChar defaultVariantName[] = wxT( "< Default >" );
 
@@ -1323,37 +1323,49 @@ int GetTrailingInt( const wxString& aStr )
 
 wxString GetIllegalFileNameWxChars()
 {
-    return From_UTF8( illegalFileNameChars );
+    return wxString::FromUTF8( illegalFileNameChars.data(), illegalFileNameChars.length() );
 }
 
 
-bool ReplaceIllegalFileNameChars( std::string* aName, int aReplaceChar )
+bool ReplaceIllegalFileNameChars( std::string& aName, int aReplaceChar )
 {
-    bool changed = false;
-    std::string result;
-    result.reserve( aName->length() );
+    size_t first_illegal_pos = aName.find_first_of( illegalFileNameChars );
 
-    for( std::string::iterator it = aName->begin();  it != aName->end();  ++it )
+    if( first_illegal_pos == std::string::npos )
     {
-        if( strchr( illegalFileNameChars, *it ) )
+        return false;
+    }
+
+    std::string result;
+    // result will be at least equal to original, add 16 in case of hex replacements
+    result.reserve( aName.length() + 16 );
+    // append the valid part
+    result.append( aName, 0, first_illegal_pos );
+
+    for( size_t i = first_illegal_pos; i < aName.length(); ++i )
+    {
+        char c = aName[i];
+
+        // Check if this specific char is illegal
+        if( illegalFileNameChars.find( c ) != std::string_view::npos )
         {
             if( aReplaceChar )
-                StrPrintf( &result, "%c", aReplaceChar );
+            {
+                result.push_back( aReplaceChar );
+            }
             else
-                StrPrintf( &result, "%%%02x", *it );
-
-            changed = true;
+            {
+                fmt::format_to( std::back_inserter( result ), "%{:02x}", static_cast<unsigned char>( c ) );
+            }
         }
         else
         {
-            result += *it;
+            result.push_back( c );
         }
     }
 
-    if( changed )
-        *aName = std::move( result );
-
-    return changed;
+    aName = std::move( result );
+    return true;
 }
 
 

eeschema/sch_io/cadstar/cadstar_sch_archive_loader.cpp

@@ -585,7 +585,7 @@ void CADSTAR_SCH_ARCHIVE_LOADER::loadSheets()
         std::string filename = wxString::Format( "%s_%02d", loadedFilePath.GetName(),
                                                  getSheetNumber( rootSheetID ) )
                                        .ToStdString();
-        ReplaceIllegalFileNameChars( &filename );
+        ReplaceIllegalFileNameChars( filename );
         filename += wxT( "." ) + wxString( FILEEXT::KiCadSchematicFileExtension );
 
         wxFileName fn( m_schematic->Project().GetProjectPath() + filename );
@@ -2481,7 +2481,7 @@ void CADSTAR_SCH_ARCHIVE_LOADER::loadSheetAndChildSheets( const LAYER_ID&
     wxString    loadedFilename = wxFileName( Filename ).GetName();
     std::string filename = wxString::Format( "%s_%02d", loadedFilename, sheetNum ).ToStdString();
 
-    ReplaceIllegalFileNameChars( &filename );
+    ReplaceIllegalFileNameChars( filename );
     filename += wxT( "." ) + wxString( FILEEXT::KiCadSchematicFileExtension );
 
     sheet->GetField( FIELD_T::SHEET_FILENAME )->SetText( filename );

eeschema/sch_io/eagle/sch_io_eagle.cpp

@@ -852,7 +852,7 @@ void SCH_IO_EAGLE::loadSheet( const std::unique_ptr<ESHEET>& aSheet )
         else
             sheet->SetName( filename );
 
-        ReplaceIllegalFileNameChars( &filename );
+        ReplaceIllegalFileNameChars( filename );
         replace( filename.begin(), filename.end(), ' ', '_' );
 
         fn.SetName( filename );

include/string_utils.h

@@ -286,7 +286,7 @@ KICOMMON_API bool IsFullFileNameValid( const wxString& aFullFilename );
  * @param aReplaceChar (if not 0) is the replacement char.
  * @return true if any characters have been replaced in \a aName.
  */
-KICOMMON_API bool ReplaceIllegalFileNameChars( std::string* aName, int aReplaceChar = 0 );
+KICOMMON_API bool ReplaceIllegalFileNameChars( std::string& aName, int aReplaceChar = 0 );
 KICOMMON_API bool  ReplaceIllegalFileNameChars( wxString& aName, int aReplaceChar = 0 );
 
 

pcbnew/pcb_io/kicad_legacy/pcb_io_kicad_legacy.cpp

@@ -541,7 +541,7 @@ void PCB_IO_KICAD_LEGACY::loadAllSections( bool doAppend )
 
             // The footprint names in legacy libraries can contain the '/' and ':'
             // characters which will cause the FPID parser to choke.
-            ReplaceIllegalFileNameChars( &fpName );
+            ReplaceIllegalFileNameChars( fpName );
 
             if( !fpName.empty() )
                 fpid.Parse( fpName, true );
@@ -3171,7 +3171,7 @@ void LP_CACHE::LoadModules( LINE_READER* aReader )
 
             // The footprint names in legacy libraries can contain the '/' and ':'
             // characters which will cause the LIB_ID parser to choke.
-            ReplaceIllegalFileNameChars( &footprintName );
+            ReplaceIllegalFileNameChars( footprintName );
 
             // set the footprint name first thing, so exceptions can use name.
             fp_ptr->SetFPID( LIB_ID( wxEmptyString, footprintName ) );

qa/tests/common/CMakeLists.txt

@@ -61,6 +61,7 @@ set( QA_COMMON_SRCS
     test_reporting.cpp
     test_refdes_utils.cpp
     test_grid_helper.cpp
+    test_string_utils.cpp
     test_richio.cpp
     test_text_attributes.cpp
     text_eval/test_text_eval_parser.cpp