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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
