Hi Eric, On Thu, Nov 21, 2019 at 07:43:18AM -0500, Eric Sunshine wrote: > On Wed, Nov 20, 2019 at 2:13 PM Denton Liu <liu.denton@xxxxxxxxx> wrote: > > On Wed, Nov 20, 2019 at 01:26:04PM +0900, Junio C Hamano wrote: > > > Denton Liu <liu.denton@xxxxxxxxx> 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 > > 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.
