OK, fair point about the effort of searching for every index line. I've tested the code and it works correctly in all they ways I can think of. Newly-introduced code should be of a high standard, a good example to others, so I've got some (OK, quite a few) suggestions.
Starting in parse_mbchar_table() - Basic C
-static mbchar_table *parse_mbchar_table (char *s)
+static mbchar_table *parse_mbchar_table (const char *s)
Return value: It makes sense that if this function fails, that it
should return NULL. The table would be either be empty, or incomplete.
Returning NULL means the caller can inform the user.
- t = safe_calloc (1, sizeof (mbchar_table));
- slen = mutt_strlen (s);
- if (!slen)
- return t;
+ slen = mutt_strlen (s);
+ if (!slen)
+ return NULL;
+ t = safe_calloc (1, sizeof (mbchar_table));
Shortcuts. These two lines aren't correct, but given the short strings
we're working with, they are adequate (but need commenting).
t->chars = safe_calloc (slen, sizeof (char *));
d = t->segmented_str = safe_calloc (slen * 2, sizeof (char));
Ideally, it would be something like (pseudo-code):
int mb_len = mb_strlen (s);
t->chars = safe_calloc (mb_len, sizeof (char *));
d = t->segmented_str = safe_calloc (slen + mb_len, sizeof (char));
mbrtowc() doesn't need all the args it's given.
In particular, we don't need the converted character:
- wchar_t wc;
- k = mbrtowc (&wc, s, slen, &mbstate);
+ k = mbrtowc (NULL, s, slen, &mbstate);
In addition, we don't really need the mbstate, either (mbrtowc() will
use an internal one). If some of the characters in the string are
invalid, then we should give up and notify the user.
- k = mbrtowc (NULL, s, slen, &mbstate);
+ k = mbrtowc (NULL, s, slen, NULL);
Following on, we could simplify the failure logic:
k = mbrtowc (&wc, s, slen, &mbstate);
- if (k == 0 || k == (size_t)(-2))
- break;
- if (k == (size_t)(-1))
- {
- memset (&mbstate, 0, sizeof (mbstate));
- k = 1;
- }
+ if ((k == 0) || (k == (size_t)(-1)) || (k == (size_t)(-2)))
+ {
+ free_mbchar_table (&t);
+ return NULL;
+ }
Personal choice: Replace a while loop that uses a counter with a for loop.
- while (k > 0)
- {
- *d++ = *s++;
- k--;
- slen--;
- }
+ /* Copy the multibyte character. */
+ for (; k > 0; k--, slen--)
+ *d++ = *s++;
Comments. The function is cryptic, at best. It needs a block comment
explaining what it's doing -- splitting a multi-byte string at character
boundaries. Also there needs to be an explanation that "table->chars"
is a set of pointers into "table->segmented_str", so shouldn't be freed.
These comments should probably be repeated at the definition of "mbchar_table".
The character tables are used in three places:
hdrline: '%T' and '%Z'
status: '%r'
If the string is missing:
hdrline defaults to a space
status defaults to an empty string
A slight change in the error behaviour, here, would mean these three
calls could be factored out into a helper function, e.g.
const char *lookup_table_char (mbchar_table *t, int index)
{
if (!t)
return " ";
if ((index < 0) || (index >= t->len))
index = 0;
return t->chars[i];
}
Rich
signature.asc
Description: PGP signature
