Hey Alex,
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.
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).
We can come back and clean this up more thoroughly as a lower-priority
follow-up task.
On Wed, Oct 8, 2014 at 6:25 PM, Alex Pepper <apep...@blueshiftinc.com>
wrote:
> 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.
>
> The problem occurs when you have two options with the same argument value
> string as in:
>
> type summary add Rectangle -s "Category1" -w Category1
>
> 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.
>
>
> Here is the patch:
>
> diff --git a/source/Interpreter/Args.cpp b/source/Interpreter/Args.cpp
> index 682feb9..4dd7383 100644
> --- a/source/Interpreter/Args.cpp
> +++ b/source/Interpreter/Args.cpp
> @@ -661,6 +661,9 @@ Args::ParseOptions (Options &options)
> }
> }
>
> + // Initialize the last argument iterator so we start checking at
> beginning of args list
> + int last_argv_iter = -1;
> +
> while (1)
> {
> int long_options_index = -1;
> @@ -717,6 +720,14 @@ Args::ParseOptions (Options &options)
> argv_iter = 0;
> for (auto args_iter = m_args.begin(); args_iter !=
> m_args.end(); args_iter++, argv_iter++)
> {
> + // Skip any args we have already checked from
> previous options
> + if ( argv_iter <= last_argv_iter )
> + {
> + continue;
> + }
> + // Save the last argument we have checked
> + last_argv_iter = argv_iter;
> +
> if (*args_iter == value &&
> GetArgumentQuoteCharAtIndex(argv_iter) != '\0')
> {
> *args_iter = args_iter->substr(1);
>
>
> 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.
>
> - Alex Pepper
>
--
Todd Fiala | Software Engineer | tfi...@google.com
_______________________________________________
lldb-dev mailing list
lldb-dev@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
