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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 21, 2014, 00:40 UTC
Message-ID: <CAPig+cQpO+0hfqVKCzi1DwbosQ=smK=JiTPcYM8iP4r7VnSKjQ@mail.gmail.com>
In-Reply-To: <loom.20140320T001131-702@post.gmane.org>

On Wed, Mar 19, 2014 at 7:12 PM, Dragos Foianu <dragos.foianu@gmail.com> wrote:

Show 8 quoted lines

> Eric Sunshine <sunshine <at> sunshineco.com> writes:
>> 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 embarrassing.

It's a good illustration of the value of the review process. It's easy to overlook omissions and problems in our one's work because one reads it with the bias of knowing what it's _supposed_ to say. Reviewers (hopefully) don't have such bias: they read the code afresh.

Show 6 quoted lines

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

Agreed.

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

According to the description for #17, there are plenty of opportunities, so...

> All the best,
> Dragos