On Tue, Aug 14, 2018 at 10:10 PM Jonathan Nieder <jrnieder@xxxxxxxxx> wrote: > > Elijah Newren wrote: > > > Subject: Add missing includes and forward declares > > nit: s/declares/declarations/ Thanks. > This is a huge patch. Was it autogenerated or generated manually? > Can the commit message say something about methodology? Mostly manually. I had a simple program that would create a dummy.c file that included git-compat-util.h then exactly one header, compile it, and spit any compile errors at me. I repeated that through the top-level headers. 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? > Is there an easy way to review it? (Keep in mind that I'm super lazy. > ;-)) I guess I could send you my hacky python script that loops through the top-level header files and creates the dummy two-line c file, and you could inspect it and run it. But that only verifies that it compiles, not that the changes I choose are "correct". > > > Signed-off-by: Elijah Newren <newren@xxxxxxxxx> > > --- > [...] > > --- a/alloc.h > > +++ b/alloc.h > > @@ -1,9 +1,11 @@ > > #ifndef ALLOC_H > > #define ALLOC_H > > > > +struct alloc_state; > > struct tree; > > struct commit; > > struct tag; > > +struct repository; > > > > void *alloc_blob_node(struct repository *r); > > That's reasonable. Going forward, is there a way to tell if some of > these forward declarations are no longer needed at some point in the > future (e.g. can clang be convinced to warn us about it)? I'm not aware of anything currently; while I could have easily missed things, projects like https://github.com/include-what-you-use/include-what-you-use (which look active and have a July 2018 date on them) make me suspect there isn't a good answer currently. > [...] > > --- a/apply.h > > +++ b/apply.h > > @@ -1,6 +1,9 @@ > > #ifndef APPLY_H > > #define APPLY_H > > > > +#include "lockfile.h" > > +#include "string-list.h" > > + > > enum apply_ws_error_action { > > Here, to avoid strange behavior, we have to be careful to make sure > the headers don't #include apply.h back. It's a pretty high-level > header so there's no risk of that *phew*. :-) > [...] > > --- a/archive.h > > +++ b/archive.h > > @@ -3,6 +3,9 @@ > > > > #include "pathspec.h" > > > > +struct object_id; > > +enum object_type; > > 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. > enum object_type is defined in cache.h, so should this #include that? We could, but we don't need the definition; a forward declaration is sufficient. > [...] > > --- 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? > [...] > > --- a/fsmonitor.h > > +++ b/fsmonitor.h > > @@ -1,6 +1,13 @@ > > #ifndef FSMONITOR_H > > #define FSMONITOR_H > > > > +#include "cache.h" > > +#include "dir.h" > > + > > +struct cache_entry; > > +struct index_state; > > +struct strbuf; > > cache_entry et al are defined in cache.h, right? Are these forward > decls needed? Good catch; they can be removed. I'm pretty sure I added the forward declarations first, then noticed it wasn't enough, added the cache.h include, and forgot to clean up. > [...] > > --- a/merge-recursive.h > > +++ b/merge-recursive.h > > @@ -1,8 +1,10 @@ > > #ifndef MERGE_RECURSIVE_H > > #define MERGE_RECURSIVE_H > > > > -#include "unpack-trees.h" > > #include "string-list.h" > > +#include "unpack-trees.h" > > just curious, no need to change: where does this reordering come from? Well, since I was manually editing anyway, I saw these and decided to alphabetize it since it's a file I deal with a lot. *shrug* > [...] > > --- 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 */ But Peff requested that I remove the comments. [1] https://public-inbox.org/git/20180811043218.31456-2-newren@xxxxxxxxx/ > The rest looks good. > > Thanks and hope that helps, > Jonathan Thanks for taking a look!
