On Fri, Apr 20, 2018 at 02:42:37PM +0200, Julia Lawall wrote: > > > 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. In fact you'd already fixed it in a later patch, so just need to move the hunks about. > > > + /* 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. Hrm. AIUI the copies of the srcpos stored in nodes/properties are owned by those nodes and properties and shoulnd't be shared with anything. And the only caller of srcpos_extend() is, I believe, merging two nodes, so the input nodes should be logically discarded. So I think it should be ok. But, yeah, lifetime management is not very easy to follow in dtc, so do check. -- 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
Attachment:
signature.asc
Description: PGP signature