On Fri, 20 Apr 2018, David Gibson wrote:
> On Fri, Feb 02, 2018 at 09:41:49PM +0100, Julia Lawall wrote:
> > The parser is extended to record positions, in build_node,
> > build_node_delete, and build_property.
> >
> > srcpos structures are added to the property and node types, and to the
> > parameter lists of the above functions that construct these types.
> > Nodes and properties that are created by the compiler rather than from
> > parsing source code have NULL as the srcpos value.
> >
> > merge_nodes, defined in livetree.c uses srcpos_extend to combine
> > multiple positions, resulting in a list of positions. srcpos_extend is
> > defined in srcpos.c and removes duplicates. As srcpos_extend is
> > performing a union, it is O(n^2). The recursion serves to copy the new
> > list and then add it to the front of the old one. Using recursion keeps
> > the positions in order from newer to older. On each recursive call, the
> > current element is compared against each element in the proposed new
> > tail, to avoid duplicates. Since the list of positions is only extended
> > in srcpos_extend, and srcpos_extend does not produce lists with
> > duplicates, it is not necessary to check for the current element in the
> > tail of its own list.
> >
> > Another change to srcpos.c is to make srcpos_copy always do a full copy,
> > including a copy of the file substructure. This is required because
> > when dtc is used on the output of cpp, the successive detected file
> > names overwrite the file name in the file structure. The next field
> > does not need to be deep copied, because it is only updated in newly
> > copied positions and the positions to which it points have also been
> > copied. File names are only updated in uncopied position structures.
> >
> > Signed-off-by: Julia Lawall <Julia.Lawall@xxxxxxx>
> > Signed-off-by: Frank Rowand <frank.rowand@xxxxxxxx>
>
> Sorry I've taken so long to look at this properly.
>
> This looks pretty good overall. Couple of nits noted below..
>
> [snip]
> > diff --git a/srcpos.c b/srcpos.c
> > index 9dd3297..c6688e7 100644
> > --- a/srcpos.c
> > +++ b/srcpos.c
> > @@ -207,6 +207,7 @@ struct srcpos srcpos_empty = {
> > .last_line = 0,
> > .last_column = 0,
> > .file = NULL,
> > + .next = NULL,
> > };
> >
> > void srcpos_update(struct srcpos *pos, const char *text, int len)
> > @@ -234,13 +235,52 @@ struct srcpos *
> > srcpos_copy(struct srcpos *pos)
> > {
> > struct srcpos *pos_new;
> > + struct srcfile_state *srcfile_state;
> > +
> > + if (!pos)
> > + return NULL;
> >
> > pos_new = xmalloc(sizeof(struct srcpos));
> > memcpy(pos_new, pos, sizeof(struct srcpos));
> >
> > + if (pos_new) {
>
> No need for the if - that's what xmalloc is for (plus we've already
> written to the pointer in the memcpy().
Thanks. I'll fix it up.
> > + /* allocate without free */
> > + srcfile_state = xmalloc(sizeof(struct srcfile_state));
> > + memcpy(srcfile_state, pos->file, sizeof(struct srcfile_state));
> > +
> > + pos_new->file = srcfile_state;
> > + }
> > +
> > return pos_new;
> > }
> >
> > +static bool same_pos(struct srcpos *p1, struct srcpos *p2)
> > +{
> > + return (p1->first_line == p2->first_line &&
> > + p1->first_column == p2->first_column &&
> > + p1->last_line == p2->last_line &&
> > + p1->last_column == p2->last_column &&
> > + !strcmp(p1->file->name, p2->file->name));
> > +}
> > +
> > +struct srcpos *srcpos_extend(struct srcpos *pos, struct srcpos *newtail)
> > +{
> > + struct srcpos *next, *p;
> > +
> > + if (!pos)
> > + return newtail;
> > +
> > + next = srcpos_extend(pos->next, newtail);
> > +
> > + for (p = newtail; p != NULL; p = p->next)
> > + if (same_pos(pos, p))
> > + return next;
> > +
> > + pos = srcpos_copy(pos);
> > + pos->next = next;
> > + return pos;
>
> This copies pos and doesn't free the original - and the caller doesn't
> free the original either, so there's a leak here.
I suspect that we can't free it, because someone else may be using it, but
I will check on it.
Thanks for the review.
julia
>
> > +}
> > +
> > char *
> > srcpos_string(struct srcpos *pos)
> > {
> > diff --git a/srcpos.h b/srcpos.h
> > index 9ded12a..d88e7cb 100644
> > --- a/srcpos.h
> > +++ b/srcpos.h
> > @@ -74,6 +74,7 @@ struct srcpos {
> > int last_line;
> > int last_column;
> > struct srcfile_state *file;
> > + struct srcpos *next;
> > };
> >
> > #define YYLTYPE struct srcpos
> > @@ -105,6 +106,8 @@ extern struct srcpos srcpos_empty;
> >
> > extern void srcpos_update(struct srcpos *pos, const char *text, int len);
> > extern struct srcpos *srcpos_copy(struct srcpos *pos);
> > +extern struct srcpos *srcpos_extend(struct srcpos *new_srcpos,
> > + struct srcpos *old_srcpos);
> > extern char *srcpos_string(struct srcpos *pos);
> >
> > extern void PRINTF(3, 0) srcpos_verror(struct srcpos *pos, const char *prefix,
>
> --
> David Gibson | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
