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