All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Kastrup <dak@gnu.org>
To: git@vger.kernel.org
Subject: Re: [PATCH] blame.c: prepare_lines should not call xrealloc for every line
Date: Thu, 06 Feb 2014 01:34:51 +0100	[thread overview]
Message-ID: <87r47hjcyc.fsf@fencepost.gnu.org> (raw)
In-Reply-To: xmqqk3d92t0z.fsf@gitster.dls.corp.google.com

Junio C Hamano <gitster@pobox.com> writes:

> David Kastrup <dak@gnu.org> writes:
>
>> It's snake oil making debugging harder.
>
> OK, that is a sensible argument.
>
>>> This was fun ;-)
>>
>> At the expense of seriously impacting my motivation to do any further
>> code cleanup on Git.
>
> Well, I said it was "fun" because I was learning from a new person
> who haven't made much technical or code aethesitics discussion here
> so far.  If teaching others frustrates you and stops contributing to
> the project, that is a loss.

The implication of "Thanks for catching them, but this patch needs heavy
style fixes." is not one of learning.

> But the styles matter, as the known pattern in the existing code is
> what lets our eyes coast over unimportant details of the code while
> reviewing and understanding.  I tend to be pickier when reviewing code
> from new people who are going to make large contributions for exactly
> that reason---new blood bringing in new ideas is fine, but I'd want to
> see those new ideas backed by solid thiniking, and that means they may
> have to explain themselves to those who are new to their ideas.

Well, the point of stylistic decisions is exactly that they are not
individually "backed by solid thinking": if they were, one would not
require a style.

A pattern of
some loop {
  ...
  if (condition) {
    code intimately related to condition;
    continue;
  }
  break;
}

is somewhat awkward.  The superficially simpler

some loop {
  ...
  if (!condition)
    break;
  code intimately related to condition;
}

separates related parts with a central loop exit.  Which maybe a tossup.
The former pattern of break; at the end of a loop, however, becomes
indispensible in the form

some loop {
  ...
  switch (...) {
    various cases ending in either break; or continue;
  }
  break;
}

In this case, the break; before the end of the loop establishes the
statement "any commencement of the loop will be explicitly done using
continue;", particularly important since a break; inside of the switch
statement does not, without this help, break out of the loop.

It's a pattern that is transparent enough to be still preferable over
"crank out the goto already, chicken".

Is "if I have to use x in some situations anyway I may as well pick it
when there would be equivalent solutions" solid thinking?  Not really.
It's about choosing one's familiars.  Which ultimately boils down to
personal style.  And where the differences are not really significant
for understanding and _are_ a conscious expression rather than just an
accident, the thin line between "write in a way that does not go against
our style and/or good sense" and "write in the way I would have written
it" may be the line between fun and work.

-- 
David Kastrup

  reply	other threads:[~2014-02-06  0:35 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-02-04 20:06 [PATCH] blame.c: prepare_lines should not call xrealloc for every line David Kastrup
2014-02-04 20:10 ` David Kastrup
2014-02-04 20:49   ` Junio C Hamano
2014-02-04 21:00     ` Junio C Hamano
2014-02-04 21:09       ` David Kastrup
2014-02-04 22:28         ` Philip Oakley
2014-02-04 22:48           ` Philip Oakley
2014-02-04 20:24 ` Junio C Hamano
2014-02-04 20:52   ` David Kastrup
2014-02-04 21:03     ` Junio C Hamano
2014-02-04 21:11       ` David Kastrup
2014-02-04 21:41         ` Junio C Hamano
2014-02-04 21:27   ` David Kastrup
2014-02-04 21:44     ` Junio C Hamano
2014-02-04 21:48       ` David Kastrup
2014-02-04 22:06         ` Junio C Hamano
2014-02-05  8:39           ` David Kastrup
2014-02-05 20:39             ` Junio C Hamano
2014-02-06  0:34               ` David Kastrup [this message]
2014-02-06 10:29               ` David Kastrup
2014-02-05  9:22   ` David Kastrup
2014-02-05 20:34     ` Junio C Hamano
2014-02-05 23:45       ` David Kastrup
  -- strict thread matches above, loose matches on Subject: below --
2014-02-04 21:40 David Kastrup
2014-02-04 21:46 David Kastrup
2014-02-12 14:27 David Kastrup
2014-02-12 19:36 ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87r47hjcyc.fsf@fencepost.gnu.org \
    --to=dak@gnu.org \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.