Re: [PATCHv3 1/6] Add missing includes and forward declares
From: Jonathan Nieder <hidden>
Date: 2018-08-15 05:13:18
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 <redacted> ---
[...]
quoted hunk ↗ jump to hunk
--- 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)? [...]
quoted hunk ↗ jump to hunk
--- 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*. [...]
quoted hunk ↗ jump to hunk
--- 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? [...]
quoted hunk ↗ jump to hunk
--- 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. [...]
quoted hunk ↗ jump to hunk
--- 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? [...]
quoted hunk ↗ jump to hunk
--- 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? [...]
quoted hunk ↗ jump to hunk
--- 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