On Mon, Aug 27, 2012 at 10:55 AM, <cm...@google.com> wrote: > 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?
I think the idea is to put the declarations into gcov-io.h since they are no longer static. But you could leave the implementation in pmu-profile.c Teresa > >> David > > >> > >> > http://codereview.appspot.com/6427063/ > > > > > http://codereview.appspot.com/6427063/ -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413