Re: [PATCHv2] branch.c: simplify chain of if statements

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Eric Sunshine <sunshine <at> sunshineco.com> writes:

> 
> Other submissions have computed this value mathematically without need
> for conditionals. For instance, we've seen:
> 
>     index = (!!origin << 0) + (!!remote_is_branch << 1) + (!!rebasing << 2)
> 
> as, well as the equivalent:
> 
>     index = !!origin + !!remote_is_branch * 2 + !!rebasing * 4
> 
> Although this works, it does place greater cognitive demands on the
> reader by requiring more effort to figure out what is going on and how
> it relates to table position. The original (ungainly) chain of 'if'
> statements in the original code does not suffer this problem. It
> likewise is harder to understand than merely indexing into a
> multi-dimension table where each variable is a key.

I have seen other submissions using this logic, but I didn't think it
accomplished the goal of the patch - simplifying the code. Not that my
approach does either, but I found it a little easier to understand.

Indexing into a table like this is always going to have this problem so this
is probably not the right approach to accomplishing the microproject's goals.

> It's possible to simplify this logic and have only a single
> printf_ln() invocation. Hint: It's safe to pass in more arguments than
> there are %s directives in the format string.

Indeed. It's a habit of mine to pass the exact number of arguments to printf
functions and I can't seem to get away from it.

> You can use ARRAY_SIZE() in place of sizeof(...)/sizeof(...).
> 
> Since an out-of-bound index would be a programmer bug, it would
> probably be more appropriate to use an assert(), just after 'index' is
> computed, rather than if+die(). The original code used die() because
> it couldn't detect the error until the end of the if-chain.

Thank you for this hint. Using already defined helpers in the project is
better and will prevent the need to patch the constructs later on.

> On Tue, Mar 18, 2014 at 6:31 PM, Eric Sunshine <sunshine <at>
sunshineco.com> wrote:
> 
> One other observation: You have a one-off error in your out-of-bounds
> check. It should be 'index >= sizeof...'

Well this is embarrasing.

Thank you again for the feedback. It's incredibily helpful and I learned a
lot from submitting these patches. Making the code simple is harder than it
appears at first sight.

I'm not sure it's worth pursuing the table approach further, especially
since a solution has already been accepted and merged into the codebase.

In this case, is it okay to try another microproject? I was thinking about
trying #17 (finding bugs/inefficiencies in builtin/apply.c), but I've
already had my one microproject.

All the best,
Dragos

--
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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]