On Fri, Aug 24, 2012 at 3:56 PM, <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. > > > 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. David > > http://codereview.appspot.com/6427063/