On 06/22/2018 06:09 PM, Jeff Law wrote:
> 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
>
Thanks for the review. I hope I included all David's notes and I'm
going to install the patch. I'm planning to incrementally add DejaGNU
tests for the functionality.
Martin
