Re: tb/ban-strtok (was: Re: What's cooking in git.git (Apr 2023, #06; Thu, 20))

From: Jeff King <peff@peff.net>
Date: Apr 22, 2023, 11:22 UTC
Message-ID: <20230422112213.GE2969939@coredump.intra.peff.net>
In-Reply-To: <ZEHyBbKN5MuxqfQn@nand.local>

On Thu, Apr 20, 2023 at 10:16:37PM -0400, Taylor Blau wrote:

Show 20 quoted lines

> On Thu, Apr 20, 2023 at 03:34:26PM -0700, Junio C Hamano wrote:
> > * tb/ban-strtok (2023-04-18) 6 commits
> >  - banned.h: mark `strtok()` as banned
> >  - t/helper/test-json-writer.c: avoid using `strtok()`
> >  - t/helper/test-oidmap.c: avoid using `strtok()`
> >  - t/helper/test-hashmap.c: avoid using `strtok()`
> >  - string-list: introduce `string_list_setlen()`
> >  - string-list: introduce `string_list_split_in_place_multi()`
> >
> >  Mark strtok() and strtok_r() to be banned.
> 
> The latest round bans only strtok(), so this description would need
> updating (probably to something as simple as "Mark strtok() as banned").
> 
> >  Comments?
> 
> I would be curious to get Peff's (cc'd) thoughts on this series, since
> it was something that he and I were talking about off-list. It was one
> of those "let me see how hard this would be..." topics, that by the time
> I finished investigating, I had the series ready to go.

I left a few small comments on the series.

On the greater question of "should strtok or strtok_r be banned", I don't have too strong a feeling. The hidden global state in strtok() is bad, so probably worth outlawing.

I tend to think that strtok_r() is a bit confusing to use. As Chris noted, strsep() is better, but not necessarily as portable. Using ptr/len pairs to parse via strcspn(), etc, seems better still. But that is mostly aesthetics and preference, so I'm OK if we don't outright ban strtok_r().

-Peff