Mail Archives: geda-user/2012/06/14/18:45:43
--f46d04088c759e34c304c2767315
Content-Type: text/plain; charset=ISO-8859-1
On Thu, Jun 14, 2012 at 2:41 PM, Andrew Poelstra <asp11 AT sfu DOT ca> wrote:
> I have made the following changes to your patch:
>
>
> 1. Consolidated all 17 patches into one. They all fundamentally do
> a single thing (add a color-changing feature), all affect the same
> source file, and the total diff is only ~300 lines. I think this is
> reasonable.
>
> The 17-patch version was over 1000 lines, and largely redundant.
> (For example, one patch introduced a compilation warning, which
> the next patch got rid of.)
>
Makes sense. No one needs to see my intermediate, buggy commits.
2. Fixed whitespace. There should be no tabs in source files -- the
> ones that are there were put in by errant tools, and represent
> 8 spaces. So if you're editing code with tabs in it, check that
> your editor has its tabstops set to 8 spaces, so that the spacing
> will look right on your machine.
>
> (Or even better, submit a separate whitespace-fixing patch to get
> rid of the tabs.)
>
Good to know. I use vim and have it set to convert tabs to 2 space
characters. A vim modeline at the end of the file would help other vim
users to maintain correct indentation. Perhaps useful for another future
patch.
> 3. Changed the clip() function to check for both values > 255 and
> values < 0. Removed the explicit check for < 0 from subtract().
> This reduces the potential for bugs.
>
Makes sense to me.
The new patch is as follows. If you're happy to have your name on
> it, let me know and I'll push it.
>
I didn't read it word for word, but based on your summaries, I'd be happy
to see it in the official pcb repository :) Thanks for reviewing it.
Ben
--f46d04088c759e34c304c2767315
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
<div class=3D"gmail_quote">On Thu, Jun 14, 2012 at 2:41 PM, Andrew Poelstra=
<span dir=3D"ltr"><<a href=3D"mailto:asp11 AT sfu DOT ca" target=3D"_blank">as=
p11 AT sfu DOT ca</a>></span> wrote:<br><blockquote class=3D"gmail_quote" style=
=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I have made the following changes to your patch:<br>
<br>
<br>
=A01. Consolidated all 17 patches into one. They all fundamentally do<br>
=A0 =A0 a single thing (add a color-changing feature), all affect the same=
<br>
=A0 =A0 source file, and the total diff is only ~300 lines. I think this i=
s<br>
=A0 =A0 reasonable.<br>
<br>
=A0 =A0 The 17-patch version was over 1000 lines, and largely redundant.<b=
r>
=A0 =A0 (For example, one patch introduced a compilation warning, which<br=
>
=A0 =A0 =A0the next patch got rid of.)<br></blockquote><div><br></div><div=
>Makes sense. No one needs to see my intermediate, buggy commits.=A0</div><=
div><br></div><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;=
border-left:1px #ccc solid;padding-left:1ex">
=A02. Fixed whitespace. There should be no tabs in source files -- the<br>
=A0 =A0 ones that are there were put in by errant tools, and represent<br>
=A0 =A0 8 spaces. So if you're editing code with tabs in it, check tha=
t<br>
=A0 =A0 your editor has its tabstops set to 8 spaces, so that the spacing<=
br>
=A0 =A0 will look right on your machine.<br>
<br>
=A0 =A0 (Or even better, submit a separate whitespace-fixing patch to get<=
br>
=A0 =A0 =A0rid of the tabs.)<br></blockquote><div><br></div><div>Good to k=
now. I use vim and have it set to convert tabs to 2 space characters. A vim=
modeline at the end of the file would help other vim users to maintain cor=
rect indentation. Perhaps useful for another future patch.</div>
<div>=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;=
border-left:1px #ccc solid;padding-left:1ex">
=A03. Changed the clip() function to check for both values > 255 and<br=
>
=A0 =A0 values < 0. Removed the explicit check for < 0 from subtract=
().<br>
=A0 =A0 This reduces the potential for bugs.<br></blockquote><div><br></di=
v><div>Makes sense to me.=A0</div><div><br></div><blockquote class=3D"gmail=
_quote" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:=
1ex">
The new patch is as follows. If you're happy to have your name on<br>
it, let me know and I'll push it.<br></blockquote><div><br></div><div>I=
didn't read it word for word, but based on your summaries, I'd be =
happy to see it in the official pcb repository :) =A0 Thanks for reviewing =
it.</div>
<div><br></div><div>Ben</div></div>
--f46d04088c759e34c304c2767315--
- Raw text -