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.

[...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;
?

If so, then this dtor's body could simply be:

 option_proposer::~option_proposer ()
 {
   delete m_option_suggestions;
 }

[...snip...]

Everything else looks sane to me (I'm not a reviewer, but I
wrote much of the code you're touching).

Dave

Reply via email to