From c939b1ef764dd1b85f7281f827cc4d8453fc3ac2 Mon Sep 17 00:00:00 2001
From: Jeff Young <jeff@rokeby.ie>
Date: Mon, 30 Jan 2023 21:09:36 +0000
Subject: [PATCH] Followed-by-3-digits doesn't guarantee a thousands separator.

Fixes https://gitlab.com/kicad/code/kicad/issues/13708
---
 eeschema/sim/sim_model.cpp                    | 83 +++++++++++++++----
 .../eeschema/sim/test_sim_model_inference.cpp |  5 +-
 2 files changed, 73 insertions(+), 15 deletions(-)

diff --git a/eeschema/sim/sim_model.cpp b/eeschema/sim/sim_model.cpp
index 22890407dc..ad574d41dc 100644
--- a/eeschema/sim/sim_model.cpp
+++ b/eeschema/sim/sim_model.cpp
@@ -1131,43 +1131,98 @@ bool SIM_MODEL::InferSimModel( T_symbol& aSymbol, std::vector<T_field>* aFields,
             {
                 mantissa->Replace( wxS( " " ), wxEmptyString );
 
+                wxChar ambiguousSeparator = '?';
                 wxChar thousandsSeparator = '?';
+                bool   thousandsSeparatorFound = false;
                 wxChar decimalSeparator = '?';
-                int    length = (int) mantissa->length();
-                int    radix = length;
+                bool   decimalSeparatorFound = false;
+                int    digits = 0;
 
-                for( int ii = length - 1; ii >= 0; --ii )
+                for( int ii = (int) mantissa->length() - 1; ii >= 0; --ii )
                 {
                     wxChar c = mantissa->GetChar( ii );
 
-                    if( c == '.' || c == ',' )
+                    if( c >= '0' && c <= '9' )
                     {
-                        if( ( radix - ii ) % 4 == 0 )
+                        digits += 1;
+                    }
+                    else if( c == '.' || c == ',' )
+                    {
+                        if( decimalSeparator != '?' || thousandsSeparator != '?' )
                         {
-                            if( thousandsSeparator == '?' )
+                            // We've previously found a non-ambiguous separator...
+
+                            if( c == decimalSeparator )
                             {
-                                thousandsSeparator = c;
+                                if( thousandsSeparatorFound )
+                                    return false;       // decimal before thousands
+                                else if( decimalSeparatorFound )
+                                    return false;       // more than one decimal
+                                else
+                                    decimalSeparatorFound = true;
+                            }
+                            else if( c == thousandsSeparator )
+                            {
+                                if( digits != 3 )
+                                    return false;       // thousands not followed by 3 digits
+                                else
+                                    thousandsSeparatorFound = true;
+                            }
+                        }
+                        else if( ambiguousSeparator != '?' )
+                        {
+                            // We've previously found a separator, but we don't know for sure
+                            // which...
+
+                            if( c == ambiguousSeparator )
+                            {
+                                // They both must be thousands separators
+                                thousandsSeparator = ambiguousSeparator;
+                                thousandsSeparatorFound = true;
                                 decimalSeparator = c == '.' ? ',' : '.';
                             }
-                            else if( thousandsSeparator != c )
+                            else
                             {
-                                return false;
+                                // The first must have been a decimal, and this must be a
+                                // thousands.
+                                decimalSeparator = ambiguousSeparator;
+                                decimalSeparatorFound = true;
+                                thousandsSeparator = c;
+                                thousandsSeparatorFound = true;
                             }
                         }
                         else
                         {
-                            if( decimalSeparator == '?' )
+                            // This is the first separator...
+
+                            // If it's followed by 3 digits then it could be either.
+                            // Otherwise it -must- be a decimal separator (and the thousands
+                            // separator must be the other).
+                            if( digits == 3 )
                             {
-                                decimalSeparator = c;
-                                thousandsSeparator = c == '.' ? ',' : '.';
-                                radix = ii;
+                                ambiguousSeparator = c;
                             }
                             else
                             {
-                                return false;
+                                decimalSeparator = c;
+                                decimalSeparatorFound = true;
+                                thousandsSeparator = c == '.' ? ',' : '.';
                             }
                         }
+
+                        digits = 0;
                     }
+                    else
+                    {
+                        digits = 0;
+                    }
+                }
+
+                // If we found nothing difinitive then we have to assume SPICE-native syntax
+                if( decimalSeparator == '?' && thousandsSeparator == '?' )
+                {
+                    decimalSeparator = '.';
+                    thousandsSeparator = ',';
                 }
 
                 mantissa->Replace( thousandsSeparator, wxEmptyString );
diff --git a/qa/unittests/eeschema/sim/test_sim_model_inference.cpp b/qa/unittests/eeschema/sim/test_sim_model_inference.cpp
index 0df9848646..0fd6bde56b 100644
--- a/qa/unittests/eeschema/sim/test_sim_model_inference.cpp
+++ b/qa/unittests/eeschema/sim/test_sim_model_inference.cpp
@@ -71,12 +71,15 @@ BOOST_AUTO_TEST_CASE( InferPassiveValues )
         { wxString( "R4" ),  wxString( "3,000.5" ),      wxString( "r=\"3000.5\"" )    },
         { wxString( "R5" ),  wxString( "3.000,5" ),      wxString( "r=\"3000.5\"" )    },
         { wxString( "R6" ),  wxString( "3000.5" ),       wxString( "r=\"3000.5\"" )    },
+        { wxString( "R7" ),  wxString( "3,000K" ),       wxString( "r=\"3000K\"" )     },
+        { wxString( "R8" ),  wxString( "3.000K" ),       wxString( "r=\"3.000K\"" )    },
+        { wxString( "R9" ),  wxString( "3.0,000,000" ),  wxString( "" )                },
 
         { wxString( "X1" ),  wxString( "3.3K" ),         wxString( "" )                },
 
         { wxString( "C14" ), wxString( "33,000,000uF" ), wxString( "c=\"33000000u\"" ) },
         { wxString( "C15" ), wxString( "33 000 000uF" ), wxString( "c=\"33000000u\"" ) },
-        { wxString( "C16" ), wxString( "33.000,000uF" ), wxString( "" )                }
+        { wxString( "C16" ), wxString( "33.000,000uF" ), wxString( "c=\"33000.000u\"" )},
     };
 
     std::unique_ptr<LIB_SYMBOL> symbol = std::make_unique<LIB_SYMBOL>( "symbol", nullptr );