Re: [PATCH v2 7/8] range-diff: passthrough --[no-]notes to `git log`

From: Denton Liu <liu.denton@gmail.com>
Date: Nov 21, 2019, 18:35 UTC
Message-ID: <20191121183539.GA99414@generichostname>
In-Reply-To: <CAPig+cQ0xO2ya=3ok=GPV2Ji2HJSFvCtokiRpWJhBAR1skqKVQ@mail.gmail.com>

Hi Eric,

On Thu, Nov 21, 2019 at 07:43:18AM -0500, Eric Sunshine wrote:

Show 30 quoted lines

> On Wed, Nov 20, 2019 at 2:13 PM Denton Liu <liu.denton@gmail.com> wrote:
> > On Wed, Nov 20, 2019 at 01:26:04PM +0900, Junio C Hamano wrote:
> > > Denton Liu <liu.denton@gmail.com> writes:
> > > > +#include "argv-array.h"
> > > >
> > > >  int show_range_diff(const char *range1, const char *range2,
> > > >                 int creation_factor, int dual_color,
> > > > -               struct diff_options *diffopt);
> > > > +               struct diff_options *diffopt,
> > > > +               struct argv_array *other_arg);
> > >
> > > I thought a mere use of "pointer to a struct" did not have to bring
> > > the definition of the struct into the picture?  In other words,
> > > wouldn't it be fine to leave the other_arg a pointer to an opaque
> > > structure by not including "argv-array.h" in this file?
> >
> > Without including "argv-array.h", we get the following hdr-check error:
> >
> >         $ make range-diff.hco
> >         In file included from range-diff.hcc:2:
> >         ./range-diff.h:16:14: error: declaration of 'struct argv_array' will not be visible outside of this function [-Werror,-Wvisibility]
> >                             struct argv_array *other_arg);
> 
> Did you forward-declare 'argv_array' in range-diff.h? That is, rather than:
> 
>     #include "argv-array.h"
> 
> instead say:
> 
>     struct argv_array;

*facepalm* Damn, sorry for the brainfart. I completely forgot to do that.

That being said, I'd prefer to just include the header for consistency since we already have the #include "diff.h" up there. Or alternatively, I could write a patch to change both into forward-declarations.

Personally, I prefer to avoid forward-declarations whenever possible because it adds duplication. If we have a strong preference for one or the other, should we include it in the CodingGuidelines?

Thanks,

Denton

Show 6 quoted lines

> 
> which tells the compiler that the type exists, thus allowing you to
> deal with pointers to the struct, as long as you don't try to access
> any of the struct's members in code which has seen only the forward
> declaration. You would still need to #include "argv-array.h" in
> range-diff.c, though.