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

[Jonathan Nieder <jrnieder@gmail.com>]

Elijah Newren wrote:

> Subject: Add missing includes and forward declares

nit: s/declares/declarations/

This is a huge patch.  Was it autogenerated or generated manually?
Can the commit message say something about methodology?

Is there an easy way to review it?  (Keep in mind that I'm super lazy.
;-))

> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
[...]
> --- 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)?

[...]
> --- 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.

enum object_type is defined in cache.h, so should this #include that?

[...]
> --- 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.

[...]
> --- 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?

[...]
> --- 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?

[...]
> --- 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 rest looks good.

Thanks and hope that helps,
Jonathan