Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
> What you cleverly did not mention: It was inside a
>
> if (!bad &&
> (length = strlen(source[i])) >= 0 &&
> !strncmp(destination[i], source[i], length) &&
> (destination[i][length] == 0 || destination[i][length] == '/'))
>
> construct. So, we assign the "length" variable only if we have to. And the
> ">= 0" trick is a common one. I could have done
>
> !strncmp(destination[i], source[i], (length = strlen(source[i])))
>
> but even I find that ugly.
I usually side with you but on this I can't.
There are 2 ways to generate branch instructions in C.
- compound statements specifically designed for expressing
control structure: if () ... else ..., for (), while (),
switch (), etc.
- expressions using conditional operators or logical operators
that short circuit: ... ? ... : ..., ... && ... || ...
The latter form may still be readable even with simple side
effects inside its terms, but "(l = strlen(s)) >= 0" is done
solely for the side effect, and its computed value does not have
anything to do with the logical operation &&.
THIS IS UGLY. And do not want to live in a world where this
ugliness is a "common one", as you put it.
And this avoiding one call to strlen(source[i]) is unnecessary
even as an optimization -- you end up calling strlen() on it
later in the code anyway, as David points out.
I think this part is far easier to read if you did it like this:
length = strlen(source[i]);
if (lstat(source[i], &st) < 0)
bad = "bad source";
else if (!strncmp(destination[i], source[i], length) &&
(destination[i][length] == 0 ||
destination[i][length] == '/'))
bad = "can not move directory into itself";
if (S_ISDIR(st.st_mode)) {
...
Note that the above is an absolute minimum rewrite. Other
things I noticed are:
- source[i] and destination[i] are referenced all the time; the
code would be easer to read if you had something like this
upfront:
/* Checking */
for (i = 0; i < count; i++) {
const char *bad = NULL;
const char *src = source[i];
const char *dst = destination[i];
int srclen = strlen(src);
int dstlen = strlen(dst);
You might end up not using dstlen in some cases, but I think
this would be far easier to read. Micro-optimizing by saying
"this is used only in this branch of this later if()
statement but in that case it is always set in that branch of
that earlier if() statement" makes unmaintainably confusing
code.
- I do not think you need "const char *dir, *dest_dir" inside
the "source is directory" branch; I would just use src and dst
consistently;
- You muck with dest_dir by calling add_slash(dest_dir) but
call prefix_path() with dst_len you computed earlier;
prefix_path() may know what to do, but is this intended?
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html