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
