On 2012/08/24 23:20:21, davidxl wrote:
On Fri, Aug 24, 2012 at 3:56 PM, <mailto:cm...@google.com> wrote: > > http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h > File gcc/gcov-io.h (right): > >
http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h#newcode688
> gcc/gcov-io.h:688: gcov_unsigned_t index; /* The corresponding
string
> table index */ > On 2012/08/24 22:30:30, davidxl wrote: >> >> Is this field necessary? > > > For the purposes of gcov_dump output we wanted to output not only
the
> string table entry but also its index in the string table just in
case
> there were any problems with ordering that might map filetags to the > wrong filename. Because of this and the fact that gcov.c and
gcov-dump.c
> read one entry at a time, it seemed better to have the index be > self-contained within the struct to avoid extra logic.
That is fine -- but you probably need to add assertion check when the string table is read in.
> > Also, since the string table entry has to be written with its > corresponding index for gcov-dump, the gcov_write_string_table_entry > function could have a similar syntax to the ll/brm writing
functions.
> > >
http://codereview.appspot.com/6427063/diff/11002/gcc/gcov-io.h#newcode689
> gcc/gcov-io.h:689: char* filename; /* The filename that
belongs
> at this index */ > On 2012/08/24 22:30:30, davidxl wrote: >> >> Can this field name be generalized? > > > Generalized in what way? I used this name because it was used before
in
> the old gcda format.
Since it is string table, it can be something like str_ or strval_
etc. Done. filename will be changed to str in an upcoming patch.
> > >
http://codereview.appspot.com/6427063/diff/11002/libgcc/pmu-profile.c
> File libgcc/pmu-profile.c (right): > >
http://codereview.appspot.com/6427063/diff/11002/libgcc/pmu-profile.c#newcode83
> libgcc/pmu-profile.c:83: > On 2012/08/24 22:30:30, davidxl wrote: >> >> Who is the caller to this interface? > > >> It should be declared in gcov-io.h > > > The only current caller of this is > https://critique.corp.google.com/#review/31972005. This is also the
same
> for the other two ll/brm writing functions. Should I declare those
in
> gcov-io.h as well?
Any utilities functions that may be be used by clients other than gcov tool should be in the common header.
However, I don't really know how to answer this. It seems that if I move the mentioned utility functions to gcov-io.h, I should also move their helper functions, like convert_unsigned_to_pct, to gcov-io.h as well and completely remove pmu-profile.c. Does this seem like a reasonable solution?
David
> > http://codereview.appspot.com/6427063/
http://codereview.appspot.com/6427063/