Re: updated: [54d6ec8] replaced buggy concat_dir_and_file() by mhl_str_dir_plus_file()

[Roland Illig <roland illig gmx de>]

Andrew Borodin schrieb:
> On Sat, 31 Jan 2009 18:17:03 +0100 (CET) "Enrico Weigelt, metux IT service" wrote:
>> +static inline char* mhl_str_dir_plus_file(const char* dirname, const char* filename)
>> +{
>> +    /* make sure we have valid strings */
>> +    if (!dirname)
>> +	dirname="";

Why is this necessary? The function is called "dir_plus_file", so what's
the point of passing no directory at all?

>> +
>> +    if (!filename)
>> +	filename="";

Same for the filename. I'd rather add an assert(dirname != NULL) and
assert(filename != NULL).

>> +
>> +    /* skip leading slashes on filename */
>> +    while (*filename == '/')
>> +	filename++;

If you follow the naming conventions of POSIX, a filename never contains
a slash.

http://www.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap03.html#tag_03_169

>> +
>> +    /* skip trailing slashes on dirname */
>> +    int dnlen = strlen(dirname);
>> +    while (dnlen && (dirname[dnlen-1]=='/'))
>> +	dnlen--;

Since dnlen is an int, not a boolean, you should treat it as such and
write (dnlen != 0).

>> +
>> +    int fnlen = strlen(filename);
>> +    char* buffer = mhl_mem_alloc_z(dnlen+fnlen+2);	/* enough space for dirname, /, filename, zero */
>> +    char* ptr = buffer;
>> +
>> +    memcpy(ptr, dirname, dnlen);
>> +    ptr+=dnlen;
>> +    *ptr = '/';
>> +    ptr++;

The idiom *ptr++ is so common in C that every programmer should
understand it immediately. There's usually no gain in separating these
two actions.

>> +    memcpy(ptr, filename, fnlen);
>> +    ptr+=fnlen;
>> +    *ptr = 0;

Please write '\0' to emphasize that you are working with characters, not
integers here. All the rest of the Midnight Commander codebase follows
the convention to write one space character around binary operators like
+=. It's good style to follow this convention.

>> +
>> +    return buffer;
>> +}
>> +
>>  #endif /* __MHL_STRING_H */

Oh, and by the way: Every identifier that starts with a double
underscore can result in undefined behavior.

Roland