[lldb-dev] Possible fix for Bug 21190 - CategoriesDataFormatterTestCase failure

[Todd Fiala]

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 <apepper at 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 | tfiala at google.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20141008/8ea4a34c/attachment.html>