Re: [PATCH 02/16] refs: add methods for misc ref operations
- From
David Turner <dturner@twopensource.com>
- Date
- Dec 11, 2015, 23:49 UTC
- Message-ID
- <1449877765.1678.2.camel@twopensource.com>
- In-Reply-To
- <xmqqmvtgd06p.fsf@gitster.mtv.corp.google.com>
On Fri, 2015-12-11 at 15:39 -0800, Junio C Hamano wrote:
Show 30 quoted lines
> David Turner <dturner@twopensource.com> 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).The previous review was here: http://permalink.gmane.org/gmane.comp.version-control.git/279062
Michael wrote:
Show 9 quoted lines
> 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?