On 06/22/2018 05:07 AM, Martin Liška wrote:
> On 06/20/2018 04:57 PM, David Malcolm wrote:
>> On Mon, 2018-05-14 at 14:51 +0200, Martin Liška wrote:
>>> Second part refactors function from gcc.c into a new class
>>> option_proposer.
>>>
>>> Martin
>>
>> [...snip...]
>>
>> diff --git a/gcc/c-family/cppspec.c b/gcc/c-family/cppspec.c
>> index 1e0a8bcd294..66540239f53 100644
>> --- a/gcc/c-family/cppspec.c
>> +++ b/gcc/c-family/cppspec.c
>> @@ -21,6 +21,7 @@ along with GCC; see the file COPYING3. If not see
>> #include "system.h"
>> #include "coretypes.h"
>> #include "tm.h"
>> +#include "opt-suggestions.h"
>> #include "gcc.h"
>>
>> I'm not a reviewer for this, and I don't know what our policy here is,
>> but please can we make "gcc.h" self-contained, and have it include this
>> the header it needs, rather than have every file that includes "gcc.h"
>> need to know what includes gcc.h might need. Seems like a violation of
>> the DRY principle.
>
> You mean here including "opt-suggestions.h" in "gcc.h"? I believe we have
> still the rule to not include a header from header file :/
>
>>
>> [...snip more examples of added headers...]
>>
>> [...snip changes to gcc.c and gcc.h which I can't approve but which
>> look sane to me...]
>>
>> [...snip more examples of added headers...]
>>
>> diff --git a/gcc/opt-suggestions.c b/gcc/opt-suggestions.c
>>
>> [...snip...]
>>
>> +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;
>> + }
>> +}
>>
>> Could m_option_suggestions be a
>> auto_string_vec *m_option_suggestions;
>> rather than a:
>> auto_vec <char *> *m_option_suggestions;
>
> Yes.
>
>> ?
>>
>> If so, then this dtor's body could simply be:
>>
>> option_proposer::~option_proposer ()
>> {
>> delete m_option_suggestions;
>> }
>
> Sure.
>
>>
>> [...snip...]
>>
>> Everything else looks sane to me (I'm not a reviewer, but I
>> wrote much of the code you're touching).
>>
>> Dave
>>
>
> I'm attaching updated version.
Based on David's comments, this is fine once the set as a whole is ACK'd.
jeff
