Re: [PATCH v6 03/19] fsck: Provide a function to parse fsck message IDs

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 21, 2015, 13:46 UTC
Message-ID: <de90ffde976a5fdcc580472387ca20ae@www.dscho.org>
In-Reply-To: <xmqq4mm38qbk.fsf@gitster.dls.corp.google.com>

Hi Junio,

On 2015-06-19 21:13, Junio C Hamano wrote:

Show 25 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
>> +#define MSG_ID(id, msg_type) { STR(id), FSCK_##msg_type },
>>  static struct {
>> +	const char *id_string;
>>  	int msg_type;
>>  } msg_id_info[FSCK_MSG_MAX + 1] = {
>>  	FOREACH_MSG_ID(MSG_ID)
>> -	{ -1 }
>> +	{ NULL, -1 }
>>  };
>>  #undef MSG_ID
>>
>> +static int parse_msg_id(const char *text, int len)
>> +{
>> +	int i, j;
>> +
>> +	if (len < 0)
>> +		len = strlen(text);
>> +
>> +	for (i = 0; i < FSCK_MSG_MAX; i++) {
> 
> I wonder an array without sentinel at the end with ARRAY_SIZE() may
> be a leaner way to do these, especially as this is all limited to
> this single file.

The real reason is that I cannot prevent a trailing comma with the way I construct the array, so FSCK_MSG_MAX is a natural way to support compilers that do not allow something like

    enum { A, B, C, };

Show 8 quoted lines

>> +		const char *key = msg_id_info[i].id_string;
>> +		/* match id_string case-insensitively, without underscores. */
>> +		for (j = 0; j < len; j++) {
>> +			char c = *(key++);
>> +			if (c == '_')
>> +				c = *(key++);
> 
> s/if/while/ perhaps?

Actually, I want to prevent double underscores so that no ambiguity occurs between the camelCased version of, say, JUNIO_HAMANO and JUNIO__HAMANO.

I inserted an `assert()`;

Show 8 quoted lines

>> +			if (toupper(text[j]) != c)
> 
> I know the performance would not matter very much but calling
> toupper() for each letter in the user input FSCK_MSG_MAX times
> sounds rather inefficient.
> 
> Would it make sense to make the caller upcase instead (or upcase
> upfront in the function)?

As you said, performance plays a lesser role here than simplicity. The strings we receive here are passed in read-only, therefore I would have to `strdup()` them first and then make sure to `free()` them. That was too un-simple for my taste.

Having said that, if you disagree, I will introduce the complexity, though. Do you want me to do that?

Ciao, Dscho