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

On Tue, Aug 14, 2018 at 10:10 PM Jonathan Nieder [off-list ref] wrote:

Elijah Newren wrote:

quoted

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

quoted

Signed-off-by: Elijah Newren <redacted>
---

[...]

quoted

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

[...]

quoted

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

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

[...]

quoted

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

[...]

quoted

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

[...]

quoted

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

[...]

quoted

--- 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@gmail.com/

The rest looks good.

Thanks and hope that helps,
Jonathan

Thanks for taking a look!