This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH 0/2] Levenshtein-based suggestions (v3)
- From: Marek Polacek <polacek at redhat dot com>
- To: David Malcolm <dmalcolm at redhat dot com>
- Cc: Jeff Law <law at redhat dot com>, Richard Biener <richard dot guenther at gmail dot com>, Manuel López-Ibáñez <lopezibanez at gmail dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Fri, 13 Nov 2015 16:11:19 +0100
- Subject: Re: [PATCH 0/2] Levenshtein-based suggestions (v3)
- Authentication-results: sourceware.org; auth=none
- References: <55FB150C dot 5090105 at redhat dot com> <1446209267-49800-1-git-send-email-dmalcolm at redhat dot com> <56370667 dot 8000005 at redhat dot com> <1447380516 dot 7830 dot 34 dot camel at surprise> <20151113065738 dot GH3185 at redhat dot com> <1447416968 dot 7830 dot 43 dot camel at surprise>
On Fri, Nov 13, 2015 at 07:16:08AM -0500, David Malcolm wrote:
> > > + && (TREE_CODE (TREE_TYPE (field)) == RECORD_TYPE
> > > + || TREE_CODE (TREE_TYPE (field)) == UNION_TYPE))
> >
> > This is RECORD_OR_UNION_TYPE_P (TREE_TYPE (field)).
>
> I based this code on the code in lookup_field right above it;
> I copied-and-pasted that conditional, so presumably it should also be
> changed in lookup_field (which has the condition twice)?
>
> FWIW I notice RECORD_OR_UNION_TYPE_P also covers QUAL_UNION_TYPE.
>
> /* Nonzero if TYPE is a record or union type. */
> #define RECORD_OR_UNION_TYPE_P(TYPE) \
> (TREE_CODE (TYPE) == RECORD_TYPE \
> || TREE_CODE (TYPE) == UNION_TYPE \
> || TREE_CODE (TYPE) == QUAL_UNION_TYPE)
>
> FWIW I've made the change in the attached patch (both to the new
> function, and to lookup_field).
Sorry, I changed my mind. Since QUAL_UNION_TYPE is Ada-only thing and
we check (RECORD_TYPE || UNION_TYPE) in a lot of places in the C FE,
introducing RECORD_OR_UNION_TYPE_P everywhere would unnecessarily slow
things down. I think we should have a C FE-only macro, maybe called
RECORD_OR_UNION_TYPE_P that only checks for those two types, but this is
something that I can deal with later on.
So I think please just drop these changes for now. Sorry again.
> > > + const bool debug = false;
> > > +
> > > + if (debug)
> > > + {
> > > + printf ("s: \"%s\" (len_s=%i)\n", s, len_s);
> > > + printf ("t: \"%s\" (len_t=%i)\n", t, len_t);
> > > + }
> >
> > Did you leave this debug stuff here intentionally?
>
> I find it useful, but I believe it's against our policy, so I've deleted
> it in the attached patch.
Probably. But you could surely have a separate DEBUG_FUNCTION that can be
called from gdb.
> > > + /* Build the rest of the row by considering neighbours to
> > > + the north, west and northwest. */
> > > + for (int j = 0; j < len_s; j++)
> > > + {
> > > + edit_distance_t cost = (s[j] == t[i] ? 0 : 1);
> > > + edit_distance_t deletion = v1[j] + 1;
> > > + edit_distance_t insertion = v0[j + 1] + 1;
> >
> > The formatting doesn't look right here.
>
> It's correct; it's "diff" inserting two spaces before a tab combined
> with our mixed spaces+tab convention: the "for" is at column 6 (6
> spaces), whereas the other lines are at column 8 (1 tab), which looks
> weird in a diff.
Sorry, what I had in mind were the spaces after "deletion" and "insertion"
before "=". Not a big deal, of course.
> Patch attached; only tested lightly so far (compiles, and passes
> spellcheck subset of tests).
>
> OK for trunk if it passes bootstrap®rtest?
Ok modulo the RECORD_OR_UNION_TYPE_P changes, thanks.
Marek