Re: [dev] [st PATCH 1/3] xloadcols: remove cp variable

From: Garrick Peterson <garrickp_AT_gmail.com>
Date: Wed, 22 Apr 2015 08:14:36 -0600

> On Apr 22, 2015, at 1:05 AM, Roberto E. Vargas Caballero <k0ga_AT_shike2.com> wrote:
>
> mihail.zenkov:
>> IMHO in this particular case index version much better for reading and
>> understanding.
>
> So, you think that is more understable:
>
> XftColorFree(xw.dpy, xw.vis, xw.cmap, &dc.col[i]);

I believe it provides more context for what is occurring at that line than the `cp` version. Without viewing any of the surrounding code, I can make the intelligent assumptions that dc.col is an array which we’re iterating over in a for loop, that it’s part of a dc structure (which itself is not a pointer), and I can reasonably infer that this call is going to modify the array element itself, not some outside value.

With the pointer loop, you need to understand the context of the surrounding for loop to understand what this statement is executing. I can’t tell anything from just this one line of code other than that a method is being called with what appear to be a pointer (based on the assumption that it is using a standard C pointer naming convention).

For very short loops, I personally don’t think this is an issue. For very large loops (more than a screen), it may become an issue.

> for (cp = dc.col; cp < &dc.col[LEN(dc.col)]; ++cp)

Forgive my ignorance of the LEN macro, but doesn’t this rely on accessing a memory address outside of the allocated array? Wouldn’t that cause problems if you’re up against memory boundaries? I would personally write it as either
        for (cp = dc.col; cp <= &dc.col[LEN(dc.col)-1]; ++cp)
or even better
        for (cp = dc.col; cp - dc.col < LEN(dc.col); ++cp)

-G
Received on Wed Apr 22 2015 - 16:14:36 CEST