<div dir="ltr">Hey Alex,<div><br></div><div>The option parsing quotes handling changes started when Tong was trying to get the expression parser compiler args setting introduced. So I think the ultimate cleaner fix will end up needing to look at the compiler args he couldn't initially pass in, use that as a test case for a robust solution, and also use the issue you uncovered here as another case.</div><div><br></div><div>I'd be in favor of a fix like what you have as a short-term stop-gap to address the immediate bug it is producing. (Since you found a test case where it fails, this would be a great item to put a Python test around to protect against regressions).</div><div><br></div><div>We can come back and clean this up more thoroughly as a lower-priority follow-up task.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 8, 2014 at 6:25 PM, Alex Pepper <span dir="ltr"><<a href="mailto:apepper@blueshiftinc.com" target="_blank">apepper@blueshiftinc.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">So I found what was causing the bug and have a fix, but I don’t really like it. Ideally we would do away with the hacky add+removal of quotes but that would take some additional time to find better solution.<div><br></div><div>The problem occurs when you have two options with the same argument value string as in:</div><div><br></div><div>type summary add Rectangle -s "Category1" -w Category1</div><div><br></div><div>As it iterates through the options, the first time it removes the quotes on the first “Catagory” string by searching through the m_args list to match the argument that was returned from OptionsParser::Parse. The problem arises when it searches for the next option value, it has already removed the quotes from the first “Category1” string so it matches again, then it checks if it was flagged for quote removal and removes the first character thinking it must be a quote. The problem is there is no direct matching between the string returned from the OptionsParser::Parse and the list of args so I added in a last_argv_iter variable to track which args we have already checked. This works ok but seems like it relies on the implementation of OptionsParser::Parse to always return options in order. A better way would be to remove all of the quote hackery but that will take some additional time to understand why it was added in the first place.</div><div><br></div><div><br></div><div>Here is the patch:</div><div><br></div><div><div><font face="Menlo" size="1" color="#0042aa">diff --git a/source/Interpreter/Args.cpp b/source/Interpreter/Args.cpp</font></div><div><font face="Menlo" size="1" color="#0042aa">index 682feb9..4dd7383 100644</font></div><div><font face="Menlo" size="1" color="#0042aa">--- a/source/Interpreter/Args.cpp</font></div><div><font face="Menlo" size="1" color="#0042aa">+++ b/source/Interpreter/Args.cpp</font></div><div><font face="Menlo" size="1" color="#0042aa">@@ -661,6 +661,9 @@ Args::ParseOptions (Options &options)</font></div><div><font face="Menlo" size="1" color="#0042aa"> }</font></div><div><font face="Menlo" size="1" color="#0042aa"> }</font></div><div><font face="Menlo" size="1" color="#0042aa"> </font></div><div><font face="Menlo" size="1" color="#0042aa">+ // Initialize the last argument iterator so we start checking at beginning of args list</font></div><div><font face="Menlo" size="1" color="#0042aa">+ int last_argv_iter = -1;</font></div><div><font face="Menlo" size="1" color="#0042aa">+</font></div><div><font face="Menlo" size="1" color="#0042aa"> while (1)</font></div><div><font face="Menlo" size="1" color="#0042aa"> {</font></div><div><font face="Menlo" size="1" color="#0042aa"> int long_options_index = -1;</font></div><div><font face="Menlo" size="1" color="#0042aa">@@ -717,6 +720,14 @@ Args::ParseOptions (Options &options)</font></div><div><font face="Menlo" size="1" color="#0042aa"> argv_iter = 0;</font></div><div><font face="Menlo" size="1" color="#0042aa"> for (auto args_iter = m_args.begin(); args_iter != m_args.end(); args_iter++, argv_iter++)</font></div><div><font face="Menlo" size="1" color="#0042aa"> {</font></div><div><font face="Menlo" size="1" color="#0042aa">+ // Skip any args we have already checked from previous options</font></div><div><font face="Menlo" size="1" color="#0042aa">+ if ( argv_iter <= last_argv_iter )</font></div><div><font face="Menlo" size="1" color="#0042aa">+ {</font></div><div><font face="Menlo" size="1" color="#0042aa">+ continue;</font></div><div><font face="Menlo" size="1" color="#0042aa">+ }</font></div><div><font face="Menlo" size="1" color="#0042aa">+ // Save the last argument we have checked</font></div><div><font face="Menlo" size="1" color="#0042aa">+ last_argv_iter = argv_iter;</font></div><div><font face="Menlo" size="1" color="#0042aa">+</font></div><div><font face="Menlo" size="1" color="#0042aa"> if (*args_iter == value && GetArgumentQuoteCharAtIndex(argv_iter) != '\0')</font></div><div><font face="Menlo" size="1" color="#0042aa"> {</font></div><div><font face="Menlo" size="1" color="#0042aa"> *args_iter = args_iter->substr(1);</font></div></div><div><br></div><div><br></div><div>So in the short term we can add this in as a temporary fix unless someone has objections and we can look for a better solution longer term.</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>- Alex Pepper</div></font></span></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div dir="ltr"><table cellspacing="0" cellpadding="0" style="color:rgb(136,136,136);font-family:'Times New Roman'"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td nowrap style="border-top-style:solid;border-top-color:rgb(213,15,37);border-top-width:2px">Todd Fiala |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(51,105,232);border-top-width:2px"> Software Engineer |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(0,153,57);border-top-width:2px"> <a href="mailto:tfiala@google.com" style="color:rgb(17,85,204)" target="_blank"><span style="background-color:rgb(255,255,204);color:rgb(34,34,34);background-repeat:initial initial">tfiala@google.com</span></a></td><td nowrap style="border-top-style:solid;border-top-color:rgb(238,178,17);border-top-width:2px"><br></td></tr></tbody></table><br></div>
</div>