> On Tue, Dec 17, 2013 at 7:48 AM, Teresa Johnson <tejohn...@google.com> wrote: > > On Mon, Dec 16, 2013 at 2:48 PM, Xinliang David Li <davi...@google.com> > > wrote: > >> Ok -- gcov_write_counter and gcov_write_tag_length are qualified as > >> low level primitives for basic gcov format and probably should be kept > >> in gcov-io.c. > >> > >> gcov_rewrite is petty much libgcov runtime implementation details so I > >> think it should be moved out. gcov_write_summary is not related to > >> gcov low level format either, neither is gcov_seek. Ok for them to be > >> moved? > > > > After looking at these some more, with the idea that gcov-io.c should > > encapsulate the lower level IO routines, then I think all of these > > (including gcov_rewrite) should remain in gcov-io.c. I think > > gcov_write_summary belongs there since all of the other gcov_write_*
Yep, I think gcov_write_summary and read summary should not be split in between two directories. Similary for gcov_seek/rewrite I can see either the whole low-level I/O being abstracted away to -driver.c but currently -driver.c seem to containing higher level stuff that you apparenlty want to fork for kernel implementation instead and the actual i/o seems to remain in gcov-io.c > +GCOV_LINKAGE struct gcov_var gcov_var; If gcov_var is not used by gcov-io.c only, why its declaration remains in gcov-io.h? > Index: libgcc/libgcov.h > =================================================================== > --- libgcc/libgcov.h (revision 0) > +++ libgcc/libgcov.h (revision 0) > @@ -0,0 +1,225 @@ > +/* Header file for libgcov-*.c. > + Contributed by Rong Xu <x...@google.com>. > + Copyright (C) 2013 Free Software Foundation, Inc. I believe when the code was created by moving it from elsehwre, the copyright should say original date of gcov-io.h. > + > +#include "tconfig.h" > +#include "tsystem.h" > +#include "coretypes.h" > +#include "tm.h" > +#include "libgcc_tm.h" .... I would really like someone working on header restructuring to comment on those. I am not 100% sure what our best practices currently are in respect of including headers within headers and specially after good amount of defines like gcov-io.h gets included. > + > +#include "gcov-io.h" Otherwise the patch is OK (if header restructuring is fine and after moving gcov_var structure definition into gcov-io.c if possible). Honza