Re: [PATCHv3 1/6] Add missing includes and forward declares
On Tue, Aug 14, 2018 at 11:51 PM Elijah Newren wrote: > > [...] > > >> 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. > > > > My rationale may miss the point but the standard and some real compilers > > don't like this, unfortunately. > > > > For structs, having an incomplete type is fine, but for enums we need > > the full definition. E.g. C99 sayeth (in section 6.7.2.3 "tags") > > > > A type specifier of the form > > > > enum identifier > > > > without an enumerator list shall only appear after the type it > > specifies is complete. > > What about a type specifier of the form > enum identifier * > ? Can that kind of type specifier appear before the full definition > of the enum? (Or, alternatively, if the standard doesn't say, are > there any compilers that have a problem with that?) > > If so, we can include cache.h instead. We'll probably also have to > fix up packfile.h for the exact same issue (even the same enum name) > if that's the case. Digging a little further this morning, apparently C++ has defined a forward declaration of an enum to either be useless (because it was already defined), require an explicit size specifier, or be a compilation error. That seemed stupid to me, but a little more digging turned up http://c-faq.com/null/machexamp.html , which states that sizeof(char*) != sizeof(int*) on some platforms. That was a big surprise to me. Since an enum could be a char or int (or long or...), knowing the size of the enum thus is important to knowing the size of a pointer to an enum, so we actually do need the full enum definition (or a C++ style explicit size specifier). What a crazy world. So, I'll go with the inclusion of cache.h and also fix up packfile.h the same way. Thanks for pointing this out, Jonathan.
Re: [PATCHv3 1/6] Add missing includes and forward declares
On Tue, Aug 14, 2018 at 11:13 PM Jonathan Nieder wrote: > > Elijah Newren wrote: > > > 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? > > Yes, that sounds sensible to me. Will do. > [...] > >> 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. > > My rationale may miss the point but the standard and some real compilers > don't like this, unfortunately. > > For structs, having an incomplete type is fine, but for enums we need > the full definition. E.g. C99 sayeth (in section 6.7.2.3 "tags") > > A type specifier of the form > > enum identifier > > without an enumerator list shall only appear after the type it > specifies is complete. What about a type specifier of the form enum identifier * ? Can that kind of type specifier appear before the full definition of the enum? (Or, alternatively, if the standard doesn't say, are there any compilers that have a problem with that?) If so, we can include cache.h instead. We'll probably also have to fix up packfile.h for the exact same issue (even the same enum name) if that's the case. > [...] > >>> --- 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? > > It's orthogonal to this series but might be a good change. I think I'll leave it as #leftoverbits for someone else interested. :-) > [...] > >>> --- 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 */ > > Ah. Please remove these #includes: they're part of the standard > library that we get implicitly via git-compat-util.h. > > I was tripped up because they were in quotes instead of angle > brackets. Indeed; will do.
Re: [PATCHv3 1/6] Add missing includes and forward declares
Elijah Newren wrote: > 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? Yes, that sounds sensible to me. [...] > On Tue, Aug 14, 2018 at 10:10 PM Jonathan Nieder wrote: >> 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". I suppose leaving it as an exercise to the reviewer is not terrible. Maybe some reviewer will be inspired to make a test tool we can check in, or configuration for an existing tool like iwyu. [...] >> 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. My rationale may miss the point but the standard and some real compilers don't like this, unfortunately. For structs, having an incomplete type is fine, but for enums we need the full definition. E.g. C99 sayeth (in section 6.7.2.3 "tags") A type specifier of the form enum identifier without an enumerator list shall only appear after the type it specifies is complete. [...] >>> --- 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? It's orthogonal to this series but might be a good change. [...] >>> --- 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 */ Ah. Please remove these #includes: they're part of the standard library that we get implicitly via git-compat-util.h. I was tripped up because they were in quotes instead of angle brackets. Thanks, Jonathan
Re: [PATCHv3 1/6] Add missing includes and forward declares
On Tue, Aug 14, 2018 at 10:10 PM Jonathan Nieder 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
> > ---
> [...]
> > --- 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-new...@gmail.com/
> The rest looks good.
>
> Thanks and hope that helps,
> Jonathan
Thanks for taking a look!
Re: [PATCHv3 1/6] Add missing includes and forward declares
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
> ---
[...]
> --- 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
[PATCHv3 1/6] Add missing includes and forward declares
Signed-off-by: Elijah Newren
---
alloc.h | 2 ++
apply.h | 3 +++
archive.h | 3 +++
attr.h| 1 +
branch.h | 2 ++
bulk-checkin.h| 2 ++
column.h | 1 +
commit-graph.h| 1 +
config.h | 5 +
connected.h | 1 +
convert.h | 2 ++
csum-file.h | 2 ++
diffcore.h| 4
dir-iterator.h| 2 ++
fsck.h| 1 +
fsmonitor.h | 7 +++
gpg-interface.h | 2 ++
khash.h | 3 +++
list-objects-filter.h | 4
list-objects.h| 4
ll-merge.h| 2 ++
mailinfo.h| 2 ++
mailmap.h | 2 ++
merge-recursive.h | 4 +++-
notes-merge.h | 4
notes-utils.h | 3 +++
notes.h | 3 +++
object-store.h| 1 +
object.h | 2 ++
oidmap.h | 1 +
pack-bitmap.h | 3 +++
patch-ids.h | 6 ++
path.h| 1 +
pathspec.h| 5 +
pretty.h | 4
reachable.h | 2 ++
reflog-walk.h | 1 +
refs.h| 2 ++
remote.h | 1 +
repository.h | 2 ++
resolve-undo.h| 2 ++
revision.h| 1 +
send-pack.h | 4
sequencer.h | 5 +
shortlog.h| 2 ++
submodule.h | 10 --
tempfile.h| 1 +
trailer.h | 2 ++
tree-walk.h | 2 ++
unpack-trees.h| 5 -
url.h | 2 ++
utf8.h| 2 ++
worktree.h| 1 +
53 files changed, 138 insertions(+), 4 deletions(-)
diff --git a/alloc.h b/alloc.h
index 3e4e828db4..7a41bb9eb3 100644
--- 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);
void *alloc_tree_node(struct repository *r);
diff --git a/apply.h b/apply.h
index b80d8ba736..0b70e1f3f5 100644
--- 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 {
nowarn_ws_error,
warn_on_ws_error,
diff --git a/archive.h b/archive.h
index 1f9954f7cd..64f0451e5c 100644
--- a/archive.h
+++ b/archive.h
@@ -3,6 +3,9 @@
#include "pathspec.h"
+struct object_id;
+enum object_type;
+
struct archiver_args {
const char *base;
size_t baselen;
diff --git a/attr.h b/attr.h
index 442d464db6..3185538bda 100644
--- a/attr.h
+++ b/attr.h
@@ -7,6 +7,7 @@ struct git_attr;
/* opaque structures used internally for attribute collection */
struct all_attrs_item;
struct attr_stack;
+struct index_state;
/*
* Given a string, return the gitattribute object that
diff --git a/branch.h b/branch.h
index 473d0a93e9..7d9b330eba 100644
--- a/branch.h
+++ b/branch.h
@@ -1,6 +1,8 @@
#ifndef BRANCH_H
#define BRANCH_H
+struct strbuf;
+
/* Functions for acting on the information about branches. */
/*
diff --git a/bulk-checkin.h b/bulk-checkin.h
index a85527318b..f438f93811 100644
--- a/bulk-checkin.h
+++ b/bulk-checkin.h
@@ -4,6 +4,8 @@
#ifndef BULK_CHECKIN_H
#define BULK_CHECKIN_H
+#include "cache.h"
+
extern int index_bulk_checkin(struct object_id *oid,
int fd, size_t size, enum object_type type,
const char *path, unsigned flags);
diff --git a/column.h b/column.h
index 0a61917fa7..2567a5cf4d 100644
--- a/column.h
+++ b/column.h
@@ -36,6 +36,7 @@ static inline int column_active(unsigned int colopts)
return (colopts & COL_ENABLE_MASK) == COL_ENABLED;
}
+struct string_list;
extern void print_columns(const struct string_list *list, unsigned int colopts,
const struct column_options *opts);
diff --git a/commit-graph.h b/commit-graph.h
index 76e098934a..eea62f8c0e 100644
--- 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"
struct commit;
diff --git a/config.h b/config.h
index bb2f506b27..75ba1d45ff 100644
--- a/config.h
+++ b/config.h
@@ -1,6 +1,11 @@
#ifndef CONFIG_H
#define CONFIG_H
+#include "hashmap.h"
+#include "string-list.h"
+
+struct object_id;
+
/* git_config_parse_key() returns these negated: */
#define CONFIG_INVALID_KEY 1
#define CONFIG_NO_SECTION_OR_NAME 2
diff --git a/connected.h b/connected.h
index 322dc76372..e4c961817d 100644
--- a/connected.h
+++ b/connected.h
@@ -1,6 +1,7 @@
#ifndef CONNECTED_H
#define CONNECTED_H
+struct object_id;
struct transport;
/*
diff --git a/convert.h b/convert.h
index 01385d9288..76c654385d 100644
--- a/convert.h
+++ b/convert.h
@@ -7,6 +7,8 @@
#include "string-list.h"
struct index_state;
+struct
