Re: [Patch] libgcov.c re-factoring

[Jan Hubicka <hubicka at ucw dot cz>]

> On Tue, Dec 17, 2013 at 7:48 AM, Teresa Johnson <tejohnson@google.com> wrote:
> > On Mon, Dec 16, 2013 at 2:48 PM, Xinliang David Li <davidxl@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 <xur@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