Menu

#251 searchparse fix

Unstable
closed-fixed
nobody
None
5
2014-05-19
2014-05-18
No

Hi,

When I compile svn trunk in linux, !searchparse pattern fails.
In line 6808 of Source/script.cpp, we can see:

maxlen = (int)(source_string - src_start); // Length of previous string

but src_start is not initialised to source_string.

The following patch fixes it (and !searchparse works with it):

diff --git a/Source/script.cpp b/Source/script.cpp
index 88e8515..7cafcde 100644
--- a/Source/script.cpp
+++ b/Source/script.cpp
@@ -6793,7 +6793,7 @@ DefineList *CEXEBuild::searchParseString(const TCHAR *source_string, LineParser&
 {
   if (failParam) *failParam = 0;
   DefineList *ret = NULL;
-  const TCHAR *defout = 0, *src_start = 0, *tok;
+  const TCHAR *defout = 0, *src_start = source_string, *tok;
   int toklen = 0, maxlen;
   for (;;)
   {
1 Attachments

Discussion

  • Anders

    Anders - 2014-05-18

    Can you show us the !searchparse command that fails?

    The only problem I see with a bad maxlen is "if (maxlen < 0) break;" at the end. The first time around the loop no other places should access maxlen...

     

    Last edit: Anders 2014-07-16
  • Anders

    Anders - 2014-05-18

    Your patch breaks the "starting string" version of the error message. That could be fixed of course but it seems to me like there should be a better way of doing this.

    This is my suggestion:

    Index: Source/script.cpp
    ===================================================================
    --- Source/script.cpp   (revision 6481)
    +++ Source/script.cpp   (working copy)
    @@ -6799,7 +6799,7 @@
       {
         tok = line.gettoken_str(parmOffs++);
         const bool lasttoken = parmOffs > line.getnumtokens();
    -    if (!tok || !*tok)
    +    if (!*tok)
           tok = 0, maxlen = -1; // No more tokens to search for, save the rest of the string
         else
         {
    @@ -6823,7 +6823,8 @@
             }
           }
         }
    -    if (!*source_string || (!tok && !lasttoken)) // We did not find the requested token!
    +    if (!tok && lasttoken) break;
    +    if (!tok || !*source_string) // We did not find the requested token!
         {
           if (failParam) *failParam = ret ? ret->getnum() : 0;
           if (noErrors) break; // Caller is OK with a incomplete list of matched strings
    @@ -6832,7 +6833,6 @@
           delete ret;
           return NULL;
         }
    -    if (maxlen < 0) break;
         defout = line.gettoken_str(parmOffs++), src_start = source_string += toklen;
       }
       return ret;
    

    We don't have any good tests for !searchparse and the docs do not spell out the behavior for edge cases so I don't know if this actually breaks anything.

    Here is some test code I used:

    !macro TEST_PPSearchParse params cmp
    !verbose push 2
    !define /redef D1 ""
    !undef D1
    !define /redef D2 ""
    !undef D2
    !define /redef D3 ""
    !undef D3
    !verbose pop
    !searchparse ${params}
    !ifdef TEST_VERBOSITY
    !verbose push ${TEST_VERBOSITY}
    !endif
    !if '|${D1}|${D2}|${D3}|' != '${cmp}'
    !warning `FAIL: !searchparse ${params} : '|${D1}|${D2}|${D3}|' != '${cmp}'`
    !else
    !echo `PASS: !searchparse ${params} : '|${D1}|${D2}|${D3}|' == '${cmp}'`
    !endif
    !ifdef TEST_VERBOSITY
    !verbose pop
    !endif
    !macroend
    
    !verbose 3
    !define TEST_VERBOSITY 4
    !insertmacro TEST_PPSearchParse '/noerrors "12345" "6" D1' '|${U+24}{D1}|${U+24}{D2}|${U+24}{D3}|' ; No match, no define
    !insertmacro TEST_PPSearchParse '/noerrors "12345" "6" D1 "Fail"' '|${U+24}{D1}|${U+24}{D2}|${U+24}{D3}|' ; No match, no define
    !insertmacro TEST_PPSearchParse '/ignorecase "abc" "a" D1' '|bc|${U+24}{D2}|${U+24}{D3}|'
    !insertmacro TEST_PPSearchParse '/ignorecase "abc" "a" D1 "b"' '||${U+24}{D2}|${U+24}{D3}|' ; Empty value, should we warn about this?
    !insertmacro TEST_PPSearchParse '/ignorecase "abc" "a" D1 "c"' '|b|${U+24}{D2}|${U+24}{D3}|'
    !insertmacro TEST_PPSearchParse '/ignorecase "abcdefghij" "AB" D1 "FG" D2 "I"' '|cde|h|${U+24}{D3}|'
    !insertmacro TEST_PPSearchParse '/ignorecase "abcdefghij" "AB" D1 "FG" D2 "I" D3' '|cde|h|j|'
    !insertmacro TEST_PPSearchParse '/ignorecase "abcdefghi" "AB" D1 "FG" D2 "I" D3' '|cde|h||' ; Empty final define, should we warn about this?
    !insertmacro TEST_PPSearchParse '/noerrors /ignorecase "abcdefghi" "AB" D1 "FG" D2 "Fail"' '|cde|hi|${U+24}{D3}|'
    !insertmacro TEST_PPSearchParse '/noerrors /ignorecase "abcdefghi" "AB" D1 "FG" D2 "Fail" D3' '|cde|hi|${U+24}{D3}|'
    

    My patch passes all of these, 3.0 beta 0 barfs on two of them (One is a empty value edge case and my fix is a behavior change)

     

    Last edit: Anders 2014-05-18
  • Yaacov Akiba Slama

    Your patch seems better than mine, and of course it resolves my specific case.
    Thanks a lot.

     
  • Anders

    Anders - 2014-05-19
    • status: open --> closed-fixed
     

Log in to post a comment.