On Thu, 11 Mar 2021 at 22:38, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>
> On Thu, Mar 11, 2021 at 10:24 AM Charvi Mendiratta <charvi077@xxxxxxxxx> wrote:
> > On Thu, 11 Mar 2021 at 11:55, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> > > > + size_t len = get_alpha_len(fixup_message);
> > > > + if (len && fixup_message[len] == ':') {
> > > > + fixup_message[len++] = '\0';
> > > > + fixup_commit = fixup_message + len;
> > >
> > > An alternate -- just about as compact and perhaps more idiomatic --
> > > way to write all this without introducing the new get_alpha_len()
> > > function:
> > >
> > > char *p = fixup_mesage;
> > > while (isalpha(*p))
> > > p++;
> > > if (p > fixup_message && *p == ':') {
> > > *p = '\0';
> > > fixup_commit = p + 1;
> >
> > Earlier we had discussed[1] keeping a separate helper function, so
> > that it may re-use it later. But I agree above is easier to get and
> > compact so I think maybe it will be ok, for this patch series to
> > replace it with the above and remove the function.
>
> I don't have strong feelings one way or the other whether you should
> use a function or inline it as I showed above, and since our aim is to
> land this series rather than endlessly re-rolling it, let's not spend
> a lot of cycles worrying about it.
>
> The one thing that does bother me, however, is the name of the
> function, get_alpha_len(), which tells you (somewhat) literally what
> it does but doesn't convey to the reader its actual purpose (which is
> something we should strive for when naming functions and variables).
> In that previous discussion you referenced, Junio mentioned that a
> future sub-option might want to have punctuation in its name. If that
> ever comes about, then the name get_alpha_len() no longer makes sense
> and needs to be renamed so it doesn't become a lie. Giving the
> function a better name up front would not only help readers now to
> understand what is going on, but would also help down the road when or
> if punctuation (or numbers or whatnot) become valid in a sub-option
> name. suboption_length() is one possibility. With a slight semantic
> change, skip_suboption() or latch_suboption() are other possibilities.
> Or, if you were to open-code the loop as I did above, then you might
> have a function named is_suboption_char() and use that in place of
> isalpha().
>
> So, if you do re-roll, I wouldn't mind seeing a better name for the
> function; but the rest is subjective and not worth spending time
> refining unless you actually feel that one style has a clear advantage
> over others.
Okay, I will rename the get_alpha_len() to skip_suboption().
Thanks and Regards,
Charvi
