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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Aug 15, 2018, 05:10 UTC
Message-ID: <20180815051011.GC32543@aiede.svl.corp.google.com>
In-Reply-To: <20180813171749.10481-2-newren@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>
> ---

[...]

Show 13 quoted lines

> --- 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)?

[...]

Show 10 quoted lines

> --- 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*.

[...]

Show 8 quoted lines

> --- 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?

[...]

Show 7 quoted lines

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

[...]

Show 12 quoted lines

> --- 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?

[...]

Show 9 quoted lines

> --- 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?

[...]

Show 8 quoted lines

> --- 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