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) [...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) +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? [...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. [...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. Missing leading comment. Maybe something like: /* Populate RESULTS with bash-completions options for all parameters that begin with PARAM_PREFIX, using SEPARATOR. 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?) +{ + char separator_str[] {separator, '\0'}; Is the above initialization valid C++98, or is it a C++11-ism? + 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. */ + 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? + /* 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. 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). Hope this is constructive Dave