David Turner [off-list ref] writes:
struct ref_be {
struct ref_be *next;
const char *name;
ref_transaction_commit_fn *transaction_commit;
+
+ pack_refs_fn *pack_refs;
+ peel_ref_fn *peel_ref;
+ create_symref_fn *create_symref;
+
+ resolve_ref_unsafe_fn *resolve_ref_unsafe;
+ verify_refname_available_fn *verify_refname_available;
+ resolve_gitlink_ref_fn *resolve_gitlink_ref;
};
This may have been pointed out in the previous reviews by somebody
else, but I think it is more customary to declare a struct member
that is a pointer to a customization function without leading '*',
i.e.
typedef TYPE (*customize_fn)(ARGS);
struct vtable {
...
cutomize_fn fn;
...
};
in our codebase (cf. string_list::cmp, prio_queue::compare).
On Fri, 2015-12-11 at 15:39 -0800, Junio C Hamano wrote:
David Turner [off-list ref] writes:
quoted
struct ref_be {
struct ref_be *next;
const char *name;
ref_transaction_commit_fn *transaction_commit;
+
+ pack_refs_fn *pack_refs;
+ peel_ref_fn *peel_ref;
+ create_symref_fn *create_symref;
+
+ resolve_ref_unsafe_fn *resolve_ref_unsafe;
+ verify_refname_available_fn *verify_refname_available;
+ resolve_gitlink_ref_fn *resolve_gitlink_ref;
};
This may have been pointed out in the previous reviews by somebody
else, but I think it is more customary to declare a struct member
that is a pointer to a customization function without leading '*',
i.e.
typedef TYPE (*customize_fn)(ARGS);
struct vtable {
...
cutomize_fn fn;
...
};
in our codebase (cf. string_list::cmp, prio_queue::compare).
The previous review was here:
http://permalink.gmane.org/gmane.comp.version-control.git/279062
Michael wrote:
Hmmm, I thought our convention was to define typedefs for functions
themselves, not for the pointer-to-function; e.g.,
typedef struct ref_transaction *ref_transaction_begin_fn(struct
strbuf *err);
(which would require `struct ref_be` to be changed to
ref_transaction_begin_fn *transaction_begin;
And you agreed. So I changed it. Do you want me to change it back?
Junio C Hamano <gitster <at> pobox.com> writes:
David Turner <dturner <at> twopensource.com> writes:
quoted
struct ref_be {
struct ref_be *next;
const char *name;
ref_transaction_commit_fn *transaction_commit;
+
+ pack_refs_fn *pack_refs;
+ peel_ref_fn *peel_ref;
+ create_symref_fn *create_symref;
+
+ resolve_ref_unsafe_fn *resolve_ref_unsafe;
+ verify_refname_available_fn *verify_refname_available;
+ resolve_gitlink_ref_fn *resolve_gitlink_ref;
};
This may have been pointed out in the previous reviews by somebody
else, but I think it is more customary to declare a struct member
that is a pointer to a customization function without leading '*',
i.e.
typedef TYPE (*customize_fn)(ARGS);
struct vtable {
...
cutomize_fn fn;
...
};
in our codebase (cf. string_list::cmp, prio_queue::compare).
(LMDB author here, just passing by)
IMO you're making a mistake. You should always typedef the thing itself, not
a pointer to the thing. If you only typedef the pointer, you can only use
the typedef to declare pointers and never the thing itself. If you typedef
the actual thing, you can use the typedef to declare both the thing and
pointer to thing.
It's particularly useful to have function typedefs because later on you can
use the actual typedef to declare instances of that function type in header
files, and guarantee that your function definitions match what they're
intended to match. Otherwise, assigning a bare (*func) to something that
mismatches only generates a warning, instead of an error.
--
-- Howard Chu
CTO, Symas Corp. http://www.symas.com
Director, Highland Sun http://highlandsun.com/hyc/
Chief Architect, OpenLDAP http://www.openldap.org/project/