On Wednesday 30 March 2016 15:31:27 Fons Adriaensen wrote:
> On Tue, Mar 29, 2016 at 11:12:21AM -0400, Paul Davis wrote:
> > (1) I explained to Fons in person and to everyon on the jack-devel
> > mailing list what the issues with Fons' original patch. Robin Gareus
> > has eloquently restated the basic point, but I'll do it again: the
> > ability to REVIEW code changes is critical to long-lived projects,
> > and when a change consists of 85% or more white-space/code style
> > changes and only 15% actual functional differences, code review
> > becomes all but impossible.
>
> 1. All the places where the actual code was changed were
> clearly identified by comments.
>
> 2. When I compare the commit (42393121) that applied my patch
> with previous versions, there are *lots of* major coding
> style and whitespace changes in files that I never touched.
> This means the coding style wasn't consistent to start with.
>
> 3. When I read that same commit (which I assume reflects the
> preferred style) I see things like this:
>
> engine.c, line 3884:
> -----
> if (dstport->connections && !dstport->shared->has_mixdown) {
> jack_port_type_info_t *port_type =
> jack_port_type_info (engine, dstport);
> jack_error ("cannot make multiple connections to a
> port of" " type [%s]", port_type->type_name); return -1;
> }
>
> connection = (jack_connection_internal_t*)
> malloc (sizeof(jack_connection_internal_t));
> connection->source = srcport;
> connection->destination = dstport;
> connection->srcclient = srcclient;
> connection->dstclient = dstclient;
>
> src_id = srcport->shared->id;
> dst_id = dstport->shared->id;
> jack_lock_graph (engine);
>
> if (dstclient->control->type == ClientDriver) {
> /* Ignore output connections to drivers for
> purposes of sorting. Drivers are executed first in the sort order
> anyway, and we don't want to treat graphs such as driver -> client ->
> driver as containing feedback */
> VERBOSE (engine,
> "connect %s and %s (output)",
> srcport->shared->name,
> dstport->shared->name);
> } else if (srcclient != dstclient) {
> jack_feedcounts_t *F;
> int dir = 0;
>
> if ((F = depends_direct (srcclient, dstclient)) != 0)
> { /* We already have a direct dependency, just increment the
> connection count. */ F->fwd_count++;
> dir = 1;
> } else if ((F = depends_direct (dstclient, srcclient))
> != 0) { /* We already have a reverse direct dependency, just increment
> the reverse connection count. */ F->rev_count++;
> dir = -1;
> ----
> If this is your idea of indentation that is supposed to help
> understand the code structure, then I'm lost. This sort of thing is
> why I modify whitespace when working on a file, just to be sure I have
> the correct idea of where conditionals and loops start and end. The
> original code was full of random indentation like this.
>
> Ciao,
Phew! I'm with you 100% Fons. Stay upwind, way upwind of code like this.
For starters, where the heck are the closing } for the two else clauses
above?
Cheers, Gene Heskett
--
"There are four boxes to be used in defense of liberty:
soap, ballot, jury, and ammo. Please use in that order."
-Ed Howdershelt (Author)
Genes Web page <http://geneslinuxbox.net:6309/gene>
_______________________________________________
Linux-audio-user mailing list
Linux-audio-user@xxxxxxxxxxxxxxxxxxxx
http://lists.linuxaudio.org/listinfo/linux-audio-user
