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>
> 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,
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html