On Mon, Dec 14, 2020 at 6:52 PM Jeff King <peff@xxxxxxxx> wrote:
>
> On Mon, Dec 14, 2020 at 06:43:36PM -0800, Junio C Hamano wrote:
>
> > diff --git c/git-compat-util.h w/git-compat-util.h
> > index 7d509c5022..58cd0761be 100644
> > --- c/git-compat-util.h
> > +++ w/git-compat-util.h
> > @@ -273,7 +273,8 @@ struct itimerval {
> >
> > #ifdef NO_SETITIMER
> > static inline int setitimer(int which, const struct itimerval *value, struct itimerval *newvalue) {
> > - ; /* nothing */
> > + errno = ENOSYS;
> > + return -1; /* not implemented */
> > }
> > #endif
> >
> > Alternatively we could pretend that the call always succeeds by
> > without touching errno and returning 0. That might be safer, but I
> > dunno which one we want, and I do not have a system affected by the
> > choice.
>
> I think this is a sensible choice. Before the conversion to an inline,
> the code was removed entirely! So anybody checking the return value
> would have seen an error, and we do not have to worry much about
> breaking them.
>
> For new callers, anybody checking the return value would probably
> appreciate the warning that support for the function is optional (OTOH,
> they would probably not find out themselves, but rather when Randall
> tells them ;) ).
>
> It would be nice to have a way to warn them even on platforms that have
> setitimer(), but I can't think of an easy way.
>
> > > Aside from inlining bodies, this should not have compiled on any platform:
> > >
> > > static inline void strset_remove(struct strset *set, const char *str)
> > > {
> > > return strmap_remove(&set->map, str, 0);
> > > }
> > >
> > > What is really intended here?
> >
> > I think we should just drop "return"; a void function should be
> > called in void context without requiring a value, even if that
> > return expects no value.
>
> Yeah, I think that is right. I checked earlier iterations of the series
> to see if perhaps strmap_remove() had previously returned a value, but
> it never did in any on the list.
>
> > diff --git i/strmap.h w/strmap.h
> > index c4c104411b..1e152d832d 100644
> > --- i/strmap.h
> > +++ w/strmap.h
> > @@ -165,7 +165,7 @@ static inline int strintmap_contains(struct strintmap *map, const char *str)
> >
> > static inline void strintmap_remove(struct strintmap *map, const char *str)
> > {
> > - return strmap_remove(&map->map, str, 0);
> > + strmap_remove(&map->map, str, 0);
> > }
>
> So yeah, I think that is the right fix. +cc Elijah for any other
> insight.
Doh, sorry for the bug. Yeah, that's the exact fix I'd make. Junio,
do you want to just use the changes you've already got, or would you
like me to submit a patch removing these two 'return's?
