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

From: Dragos Foianu <dragos.foianu@gmail.com>
Date: Mar 19, 2014, 23:12 UTC
Message-ID: <loom.20140320T001131-702@post.gmane.org>
In-Reply-To: <CAPig+cQKHQFNBob18g9UmZuE_mOpF3UMCBPfSKJYEYQpk1Z_tw@mail.gmail.com>

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

Show 16 quoted lines

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

Show 6 quoted lines

> 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