Re: [PATCHv3 1/6] Add missing includes and forward declares

[Elijah Newren <newren@gmail.com>]

On Tue, Aug 14, 2018 at 11:13 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Elijah Newren wrote:
>
> > I didn't want to repeat that description in all 6 patches, since all
> > six came from that, so I put it in the cover letter.  Since patch #1
> > has most that changes though, I guess it makes sense to include it at
> > least in that one?
>
> Yes, that sounds sensible to me.

Will do.

> [...]
> >> enums are of unknown size, so forward declarations don't work for
> >> them.  See bb/pedantic for some examples.
> >
> > structs are also of unknown size; the size is irrelevant when the
> > function signature merely uses a pointer to the struct or enum.  The
> > enum forward declaration fixes a compilation bug.
>
> My rationale may miss the point but the standard and some real compilers
> don't like this, unfortunately.
>
> For structs, having an incomplete type is fine, but for enums we need
> the full definition.  E.g. C99 sayeth (in section 6.7.2.3 "tags")
>
>         A type specifier of the form
>
>                 enum identifier
>
>         without an enumerator list shall only appear after the type it
>         specifies is complete.

What about a type specifier of the form
  enum identifier *
?  Can that kind of type specifier appear before the full definition
of the enum?  (Or, alternatively, if the standard doesn't say, are
there any compilers that have a problem with that?)

If so, we can include cache.h instead.  We'll probably also have to
fix up packfile.h for the exact same issue (even the same enum name)
if that's the case.

> [...]
> >>> --- a/commit-graph.h
> >>> +++ b/commit-graph.h
> >>> @@ -4,6 +4,7 @@
> >>>  #include "git-compat-util.h"
> >>>  #include "repository.h"
> >>>  #include "string-list.h"
> >>> +#include "cache.h"
> >>
> >> We can skip the #include of git-compat-util.h since all .c files
> >> include it.
> >
> > Good point.  Should I go through and remove all the inclusions of
> > git-compat-util.h in header files?
>
> It's orthogonal to this series but might be a good change.

I think I'll leave it as #leftoverbits for someone else interested.  :-)

> [...]
> >>> --- a/pathspec.h
> >>> +++ b/pathspec.h
> >>> @@ -1,6 +1,11 @@
> >>>  #ifndef PATHSPEC_H
> >>>  #define PATHSPEC_H
> >>>
> >>> +#include "string.h"
> >>> +#include "strings.h"
> >>
> >> What are these headers?
> >
> > The original patch[1] had explanations of why I added them:
> >
> > +#include "string.h"   /* For str[n]cmp */
> > +#include "strings.h"  /* For str[n]casecmp */
>
> Ah.  Please remove these #includes: they're part of the standard
> library that we get implicitly via git-compat-util.h.
>
> I was tripped up because they were in quotes instead of angle
> brackets.

Indeed; will do.