[PATCH 3/3] Come up with new --completion option.

[Jeff Law]

On 06/22/2018 05:25 AM, Martin Liška wrote:
> On 06/20/2018 05:27 PM, David Malcolm wrote:
>> On Mon, 2018-05-14 at 14:51 +0200, Martin Liška wrote:
>>> Main part where I still need to write ChangeLog file and
>>> gcc.sh needs to be moved to bash-completions project.
>>>
>>> Martin
>>
>> As before, I'm not an official reviewer for it, but it touches code
>> that I wrote, so here goes.
>>
>> Overall looks good to me, but I have some nitpicks:
>>
>> (needs a ChangeLog)
> 
> Added.
> 
>>
>> [...snip gcc.sh change; I don't feel qualified to comment...]
>>
>> [...snip sane-looking changes to common.opt and gcc.c.  */
>>  
>> diff --git a/gcc/opt-suggestions.c b/gcc/opt-suggestions.c
>> index e322fcd91c5..2da094a5960 100644
>> --- a/gcc/opt-suggestions.c
>> +++ b/gcc/opt-suggestions.c
>> @@ -28,18 +28,6 @@ along with GCC; see the file COPYING3.  If not see
>>  #include "opt-suggestions.h"
>>  #include "selftest.h"
>>  
>> -option_proposer::~option_proposer ()
>> -{
>> -  if (m_option_suggestions)
>> -    {
>> -      int i;
>> -      char *str;
>> -      FOR_EACH_VEC_ELT (*m_option_suggestions, i, str)
>> -       free (str);
>> -      delete m_option_suggestions;
>> -    }
>> -}
>>
>> Why is this dtor going away?  If I'm reading the patch correctly,
>> option_proposer still "owns" m_option_suggestions.
>>
>> What happens if you run "make selftest-valgrind" ?  (my guess is some
>> new memory leaks)
> 
> Fixed that, should not be removed.
> 
>>  
>> +void
>> +option_proposer::get_completions (const char *option_prefix,
>> +				  auto_string_vec &results)
>>
>> Missing leading comment.  Maybe something like:
>>
>> /* Populate RESULTS with valid completions of options that begin
>>    with OPTION_PREFIX.  */
>>
>> or somesuch.
>>
>> +{
>> +  /* Bail out for an invalid input.  */
>> +  if (option_prefix == NULL || option_prefix[0] == '\0')
>> +    return;
>> +
>> +  /* Option suggestions are built without first leading dash character.  */
>> +  if (option_prefix[0] == '-')
>> +    option_prefix++;
>> +
>> +  size_t length = strlen (option_prefix);
>> +
>> +  /* Handle parameters.  */
>> +  const char *prefix = "-param";
>> +  if (length >= strlen (prefix)
>> +      && strstr (option_prefix, prefix) == option_prefix)
>>
>> Maybe reword that leading comment to
>>
>>    /* Handle OPTION_PREFIX starting with "-param".  */
>>
>> (or somesuch) to clarify the intent?
> 
> Thanks, done.
> 
>>
>> [...snip...]
>>
>> +void
>> +option_proposer::suggest_completion (const char *option_prefix)
>> +{
>>
>> Missing leading comment.  Maybe something like:
>>
>> /* Print on stdout a list of valid options that begin with OPTION_PREFIX,
>>    one per line, suitable for use by Bash completion.
>>
>>    Implementation of the "-completion=" option.  */
>>
>> or somesuch.
> 
> Likewise.
> 
>>
>> [...snip...]
>>
>> +void
>> +option_proposer::find_param_completions (const char separator,
>> +					 const char *option_prefix,
>> +					 auto_string_vec &results)
>>
>> Maybe rename "option_prefix" to "param_prefix"?  I believe it's now
>> the prefix of the param name, rather than the option.
> 
> Makes sense.
> 
>>
>> Missing leading comment.  Maybe something like:
>>
>> /* Populate RESULTS with bash-completions options for all parameters
>>    that begin with PARAM_PREFIX, using SEPARATOR.
> 
> It's in header file, thus I copied that.
> 
>>
>>    For example, if PARAM_PREFIX is "max-" and SEPARATOR is "=" then
>>    strings of the form:
>>      "--param=max-unrolled-insns"
>>      "--param=max-early-inliner-iterations"
>>    will be added to RESULTS.  */
>>
>> (did I get that right?)
> 
> Yes.
> 
>>
>> +{
>> +  char separator_str[] {separator, '\0'};
>>
>> Is the above initialization valid C++98, or is it a C++11-ism?
> 
> You are right. I fixed that and 2 more occurrences of the same
> issue.
> 
>>
>> +  size_t length = strlen (option_prefix);
>> +  for (unsigned i = 0; i < get_num_compiler_params (); ++i)
>> +    {
>> +      const char *candidate = compiler_params[i].option;
>> +      if (strlen (candidate) >= length
>> +	  && strstr (candidate, option_prefix) == candidate)
>> +	results.safe_push (concat ("--param", separator_str, candidate, NULL));
>> +    }
>> +}
>> +
>> +#if CHECKING_P
>> +
>> +namespace selftest {
>> +
>> +/* Verify that PROPOSER generates sane auto-completion suggestions
>> +   for OPTION_PREFIX.  */
>> +static void
>> +verify_autocompletions (option_proposer &proposer, const char *option_prefix)
>> +{
>> +  auto_string_vec suggestions;
>> +  proposer.get_completions (option_prefix, suggestions);
>>
>>
>> Maybe a comment here to specify:
>>
>>    /* There must be at least one suggestion, and every suggestion must
>>       indeed begin with OPTION_PREFIX.  */
> 
> Yes!
> 
>>
>> +  ASSERT_GT (suggestions.length (), 0);
>> +
>> +  for (unsigned i = 0; i < suggestions.length (); i++)
>> +    ASSERT_STR_STARTSWITH (suggestions[i], option_prefix);
>> +}
>>
>> [...snip...]
>>
>> diff --git a/gcc/opt-suggestions.h b/gcc/opt-suggestions.h
>> index 5e3ee9e2671..d0c32af7e5c 100644
>> --- a/gcc/opt-suggestions.h
>> +++ b/gcc/opt-suggestions.h
>> @@ -33,9 +33,6 @@ public:
>>    option_proposer (): m_option_suggestions (NULL)
>>    {}
>>  
>> -  /* Default destructor.  */
>> -  ~option_proposer ();
>>
>> Again, why does this dtor go away?
> 
> Fixed.
> 
>>
>>  
>> +  /* Find parameter completions for --param format with SEPARATOR.
>> +     Again, save the completions into results.  */
>> +  void find_param_completions (const char separator, const char *option_prefix,
>> +			       auto_string_vec &results);
>>
>> If we're renaming "option_prefix" to "param_prefix", please do so here as well.
> 
> Done.
> 
>>
>>  private:
>>    /* Cache with all suggestions.  */
>>    auto_vec <char *> *m_option_suggestions;
>>
>> [...snip...]
>>  
>> +/* Evaluate STRING and PREFIX and use strstr to determine if STRING
>> +   starts with PREFIX.
>> +   ::selftest::pass if starts.
>> +   ::selftest::fail if does not start.  */
>>
>> "strstr" seems like an implementation detail to me (or am I missing
>> something here?).  Maybe reword to:
>>
>>  /* Evaluate STRING and PREFIX and determine if STRING starts with PREFIX.
>>     ::selftest::pass if STRING does start with PREFIX.
>>     ::selftest::fail if does not, or either is NULL.  */
>>
>> Thanks for working on this; the rest looks good to me (though as I
>> said, I'm not officially a reviewer for this).
> 
> Thanks for the review.
> 
>>
>> Hope this is constructive
> 
> Yes, very.
So if David is OK with this version, so am I.

jeff