Mail Archives: geda-user/2016/01/03/21:12:43
--001a113aca4e7891d0052878a7e7
Content-Type: text/plain; charset=UTF-8
I'm probably starting to run out of energy for tonight, having got chatting
on IRC...
Before I get started, I'm probably going to pick out some areas, and quote
project "rules" which I'm sure I've broken myself at one time or other,
either inadvertently, deliberately, or out of laziness! - Bear that in mind
if you think I'm being unfair...
What I would also say though, is that I do very much try to hold myself to
these rules, as I did when I was working on gEDA, and PCB far more actively.
Some general themes which I'll not drag up for individual patches...
"//" comments are not PCB style.
Nor is "if () {" in the core... it is:
if (condition)
{
code();
more_code();
}
NB: What you do matches more with the GTK HID, which (for some unknown
reason) uses a slightly different coding style. Go figure. I don't like
PCB's style, but I'd ask that you respect it like every other developer has
done. We might decide to change it at some point, but I don't think one
person should force this on us with a code dump. Considering a different
coding style in a brand-new file is probably a reasonable compromise
though... if it is a huge imposition to fix areas where you've written
significant amounts of new code.
Ideally, I'd say avoid mixing white-space changes with functional ones - it
makes review harder. It is fine to change these things as you encounter
them, just do it as a separate commit. (Having stgit helps no end here).
Please don't automate a huge white-space change.. it would cause me NO END
of merge pain - lets just try to improve things gradually as we work around
certain areas of code.
FWIW, My general rule of thumb, is I'll let myself make white-space changes
within a few lines of a real code-change, if it is in the same function,
and makes things look less ugly overall. (Obviously, fixing trailing
white-space on lines with real code-changes is fine too). Anything more
invasive, ought to be a separate patch (although I'm sure you'd be able to
find commits where I've broken this "rule").
When we get to committing / merging, I'd prefer to see your simpler changes
fixed (if required), and committed (cherry-picked?) on top of master - but
its not super important.
Do you have any thoughts on deleting branches once merged? (I dumped a list
of branches, and spent a long time reviewing home/bkerin/drc_fixes before I
realised it was already merged!) I had many comments on that one (some of
which I now see you addressed afterwards)... TBH - we should really kill
off pointer warping in a modern UI... it just doesn't fit any more -
although this is probably not something which needs discussing now though.
On to the specifics:
"backtrace"
Big NAK on merging unless you can show it builds and runs on Win32. (Or can
be committed, but disabled by default).
This is not the kind of code I like carrying - developers debug under gdb
and valgrind (so it is of less use to them), and since we're good
developers - we don't ship code that crashes - right?
DJ suggested we could commit with the code disabled, which I'd be ok with.
"home/bkerin/debugging_markers"
I'm not keen on this, but don't have any strong objections.
The main ones I had:
Avoid use of assert(), especially to check bounds when "n" markers are
being added - this is ugly - you just KNOW it will fire, and cause someone
a headache.
Why maximum 1042 markers? (Why not 1043, 666, ???)
I may be a tiny bit biased, as I suspect this one will cause me merge pain
when I come to rebase my own monster pile of patches on top of it. (I've
done a fair bit of re-factoring of the HID / drawing interface).
It also strikes me as the kind of thing I'd end up wanting to pull out
(perhaps years down the line)... once we've finished using it. I personally
tend to carry debug patches like this in stgit myself - not that I'm saying
you need to do the same, but we'd be drowning in dead-code if I committed
all the ones I ever used whilst developing new features.
"home/bkerin/drc_warning_for_noobs"
This one offends my sense of OCD, to leave a DRC warning on every design,
which cannot be fixed! (But technically, it is ready for being
cherry-picked).
IMO, a better fix - if we care, is to actually run the rats-nest check
underlying the "O" command (refactor if needed), to see whether any shorts
or missing nets remain. Add DRC warning messages for "There are "n" shorted
nets in the design, see .... for details", and "There are "n" missing nets
n the design, see ... for details" - as required.
"home/bkerin/require_c99"
If you can present justification for why we want a C99 compiler, then fine
- but small branches like this probably ought just to get cherry-picked
into master, not merged.
NAK if the reason is to enable laxer coding style, without a general
decision that we want to adopt such a style. (That includes "//" comments,
and I particularly dislike defining variables mid-function scope - if it
helps, then your functions are too long!).
NB: By "general discussion", I mean - we pick a project leader, and they
decide... we can all imagine the length of mailing-list thread such a topic
could generate for approximately zero positive benefit.
I have not yet looked in depth at any of the code changes the big complex
branches, "geometry_module", "geometry_module2" and
"fix_drc_violation_locations". (Is one of the "geometry_module.." branches
now redundant??)
If these do what I think they do - then I'm looking forward to seeing the
results. (Improving the DRC output is a really important task - one which
has obviously been needed since back when we added the abstracted DRC with
the better ability to preview the violation location).
Skimming the commits in these branches, I think they might benefit from
some clean-up before merging. Bear in mind - I've not done any code-review
of the actual changes yet, I'm mostly looking at the branch structure, and
churn.
It appears that many of the patches sit upon commits duplicated in other
branches (like the debug changes) - some of which migth need changes (e.g.
no back-trace enabled by default unless it is portable), or might not want
to end up in the repository.
Looking at the commit comments, it appears that there are some points in
the branch where the source won't even build (which has always been a big
NO NO, for pushing code into master). I would expect that after each
commit, the code should build, AND be fully functional. (This really helps
with bisecting to find errors etc..).
If I were you, I would pull that entire branch into stgit ("stg uncommit -n
<number of patches>"), and start moving code about, re-factoring patches,
until the deltas for each commit are clean, all commits build (and ideally
work!).
This strategy can be time-consuming (look how long many of my patches have
sat), but I really believe it yields better code changes. This is a key
difference between working with SCM systems like git (which let you prepare
and finesse code changes), as compared to the bad-old-days of CVS, where
developers would hack away on changes in a time-linear fashion - since
managing branches and patch series was hard with those tools.
I'm still keen to do an actual code-review of the changes in these bigger
branches, but also wanted to get your thoughts on willingness to try out
stgit, and/or shuffle patches around.
I can help if required, and could - for example, put together an
alternative stack-up of patches by way of example.
Peter
On 4 January 2016 at 00:15, Britton Kerin (britton DOT kerin AT gmail DOT com) [via
geda-user AT delorie DOT com] <geda-user AT delorie DOT com> wrote:
> looks like email as I'm not able to get to computer at the moment. list
> or not either is fine with me
> On Jan 3, 2016 1:54 PM, "Peter Clifton (petercjclifton AT googlemail DOT com)
> [via geda-user AT delorie DOT com]" <geda-user AT delorie DOT com> wrote:
>
>> Hi Britton,
>>
>> I'm reading patches now, and am about on IRC.
>>
>> Currently just going through branch by branch, making some summaries -
>> which I can share with you here, on IRC, or off-list.
>>
>> Peter
>>
>>
>> On 3 January 2016 at 00:33, Britton Kerin (britton DOT kerin AT gmail DOT com) [via
>> geda-user AT delorie DOT com] <geda-user AT delorie DOT com> wrote:
>>
>>>
>>> On Sat, Jan 2, 2016 at 12:53 PM, Peter Clifton (
>>> petercjclifton AT googlemail DOT com) [via geda-user AT delorie DOT com] <
>>> geda-user AT delorie DOT com> wrote:
>>>
>>>> Excellent,
>>>>
>>>> I think doing some interactive review / chattting on IRC will probably
>>>> be a fun way to look at this. I might ask you to take a look at some of
>>>> mine too. (If we catch Bert or DJ too by any chance, that would also be
>>>> cool).
>>>>
>>>> What timezone are you in? I'm GMT, but might not be up super early
>>>> tomorrow after two busy days I've spent in London.
>>>>
>>> Alaska UTC-9. I'll set email to beep tomorrow
>>>
>>
>>
--001a113aca4e7891d0052878a7e7
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
<div dir=3D"ltr"><div><div><div><div>I'm probably starting to run out o=
f energy for tonight, having got chatting on IRC...<br><br><div>Before I ge=
t started, I'm probably going to pick out some areas, and quote project=
"rules" which I'm sure I've broken myself at one=20
time or other, either inadvertently, deliberately, or out of laziness! - Be=
ar that in mind if you think I'm being unfair...<br><br></div><div>What=
I would also say though, is that I do very much try to hold myself to thes=
e rules, as I did when I was working on gEDA, and PCB far more actively.<br=
></div><br><br></div>Some general themes which I'll not drag up for ind=
ividual patches...<br><br></div>"//" comments are not PCB style.<=
br></div>Nor is "if () {" in the core... it is:<br><br></div><div=
>if (condition)<br>=C2=A0 {<br></div><div>=C2=A0=C2=A0=C2=A0 code();<br>=C2=
=A0=C2=A0=C2=A0 more_code();<br>=C2=A0 }<br><br></div><div>NB: What you do =
matches more with the GTK HID, which (for some unknown reason) uses a sligh=
tly different coding style. Go figure. I don't like PCB's style, bu=
t I'd ask that you respect it like every other developer has done. We m=
ight decide to change it at some point, but I don't think one person sh=
ould force this on us with a code dump. Considering a different coding styl=
e in a brand-new file is probably a reasonable compromise though... if it i=
s a huge imposition to fix areas where you've written significant amoun=
ts of new code.<br><br></div><div>Ideally, I'd say avoid mixing white-s=
pace changes with functional ones - it makes review harder. It is fine to c=
hange these things as you encounter them, just do it as a separate commit. =
(Having stgit helps no end here). Please don't automate a huge white-sp=
ace change.. it would cause me NO END of merge pain - lets just try to impr=
ove things gradually as we work around certain areas of code.<br><br>FWIW, =
My general rule of thumb, is I'll let myself make white-space changes w=
ithin a few lines of a real code-change, if it is in the same function, and=
makes things look less ugly overall. (Obviously, fixing trailing white-spa=
ce on lines with real code-changes is fine too). Anything more invasive, ou=
ght to be a separate patch (although I'm sure you'd be able to find=
commits where I've broken this "rule").<br><br><br></div><di=
v>When we get to committing / merging, I'd prefer to see your simpler c=
hanges fixed (if required), and committed (cherry-picked?) on top of master=
- but its not super important.<br><br></div><div>Do you have any thoughts =
on deleting branches once merged? (I dumped a list of branches, and spent a=
long time reviewing home/bkerin/drc_fixes before I realised it was already=
merged!) I had many comments on that one (some of which I now see you addr=
essed afterwards)... TBH - we should really kill off pointer warping in a m=
odern UI... it just doesn't fit any more - although this is probably no=
t something which needs discussing now though.<br><br></div><div><br></div>=
<div>On to the specifics:<br></div><div><br><br></div><div></div><div>"=
;backtrace"<br><br>Big NAK on merging unless you can show it builds an=
d runs on Win32. (Or can be committed, but disabled by default).<br><br>Thi=
s is not the kind of code I like carrying - developers debug under gdb and =
valgrind (so it is of less use to them), and since we're good developer=
s - we don't ship code that crashes - right?<br><br>DJ suggested we cou=
ld commit with the code disabled, which I'd be ok with.<br><br><br>&quo=
t;home/bkerin/debugging_markers"<br><br>I'm not keen on this, but =
don't have any strong objections.<br><br>The main ones I had:<br></div>=
=C2=A0 Avoid use of assert(), especially to check bounds when "n"=
markers are being added - this is ugly - you just KNOW it will fire, and c=
ause someone a headache.<br><div>=C2=A0 Why maximum 1042 markers? (Why not =
1043, 666, ???)<br></div><div><br>I may be a tiny bit biased, as I suspect =
this one will cause me merge pain when I come to rebase my own monster pile=
of patches on top of it. (I've done a fair bit of re-factoring of the =
HID / drawing interface).<br><br></div><div>It also strikes me as the kind =
of thing I'd end up wanting to pull out (perhaps years down the line)..=
. once we've finished using it. I personally tend to carry debug patche=
s like this in stgit myself - not that I'm saying you need to do the sa=
me, but we'd be drowning in dead-code if I committed all the ones I eve=
r used whilst developing new features.<br><br><br></div><div><br>"home=
/bkerin/drc_warning_for_noobs"<br><br>This one offends my sense of OCD=
, to leave a DRC warning on every design, which cannot be fixed! (But techn=
ically, it is ready for being cherry-picked).<br><br></div><div>IMO, a bett=
er fix - if we care, is to actually run the rats-nest check underlying the =
"O" command (refactor if needed), to see whether any shorts or mi=
ssing nets remain. Add DRC warning messages for "There are "n&quo=
t; shorted nets in the design, see .... for details", and "There =
are "n" missing nets n the design, see ... for details" - as=
required.<br><br><br><br></div><div>"home/bkerin/require_c99"<br=
><br>If you can present justification for why we want a C99 compiler, then =
fine - but small branches like this probably ought just to get cherry-picke=
d into master, not merged.<br><br>NAK if the reason is to enable laxer codi=
ng style, without a general decision that we want to adopt such a style. (T=
hat includes "//" comments, and I particularly dislike defining v=
ariables mid-function scope - if it helps, then your functions are too long=
!).<br><br></div><div>NB: By "general discussion", I mean - we pi=
ck a project leader, and=20
they decide... we can all imagine the length of mailing-list thread such
a topic could generate for approximately zero positive benefit.<br></div><=
div><br><br><br><br></div><div>I have not yet looked in depth at any of the=
code changes the big complex branches, "geometry_module", "=
geometry_module2" and "fix_drc_violation_locations". (Is one=
of the "geometry_module.." branches now redundant??)<br><br>If t=
hese do what I think they do - then I'm looking forward to seeing the r=
esults. (Improving the DRC output is a really important task - one which ha=
s obviously been needed since back when we added the abstracted DRC with th=
e better ability to preview the violation location).<br><br>Skimming the co=
mmits in these branches, I think they might benefit from some clean-up befo=
re merging. Bear in mind - I've not done any code-review of the actual =
changes yet, I'm mostly looking at the branch structure, and churn.<br>=
<br></div><div>It appears that many of the patches sit upon commits duplica=
ted in other branches (like the debug changes) - some of which migth need c=
hanges (e.g. no back-trace enabled by default unless it is portable), or mi=
ght not want to end up in the repository.<br><br></div><div>Looking at the =
commit comments, it appears that there are some points in the branch where =
the source won't even build (which has always been a big NO NO, for pus=
hing code into master). I would expect that after each commit, the code sho=
uld build, AND be fully functional. (This really helps with bisecting to fi=
nd errors etc..).<br><br></div><div>If I were you, I would pull that entire=
branch into stgit ("stg uncommit -n <number of patches>"),=
and start moving code about, re-factoring patches, until the deltas for ea=
ch commit are clean, all commits build (and ideally work!).<br><br></div><d=
iv>This strategy can be time-consuming (look how long many of my patches ha=
ve sat), but I really believe it yields better code changes. This is a key =
difference between working with SCM systems like git (which let you prepare=
and finesse code changes), as compared to the bad-old-days of CVS, where d=
evelopers would hack away on changes in a time-linear fashion - since manag=
ing branches and patch series was hard with those tools.<br><br><br></div><=
div>I'm still keen to do an actual code-review of the changes in these =
bigger branches, but also wanted to get your thoughts on willingness to try=
out stgit, and/or shuffle patches around.<br><br></div><div>I can help if =
required, and could - for example, put together an alternative stack-up of =
patches by way of example.<br><br></div><div>Peter<br></div><div><br></div>=
</div><div class=3D"gmail_extra"><br><div class=3D"gmail_quote">On 4 Januar=
y 2016 at 00:15, Britton Kerin (<a href=3D"mailto:britton DOT kerin AT gmail DOT com">=
britton DOT kerin AT gmail DOT com</a>) [via <a href=3D"mailto:geda-user AT delorie DOT com">=
geda-user AT delorie DOT com</a>] <span dir=3D"ltr"><<a href=3D"mailto:geda-use=
r AT delorie DOT com" target=3D"_blank">geda-user AT delorie DOT com</a>></span> wrote=
:<br><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-le=
ft:1px #ccc solid;padding-left:1ex"><p dir=3D"ltr">looks like email as I=
9;m not able to get to computer at the moment.=C2=A0 list or not either is =
fine with me</p><div class=3D"HOEnZb"><div class=3D"h5">
<div class=3D"gmail_quote">On Jan 3, 2016 1:54 PM, "Peter Clifton (<a =
href=3D"mailto:petercjclifton AT googlemail DOT com" target=3D"_blank">petercjclif=
ton AT googlemail DOT com</a>) [via <a href=3D"mailto:geda-user AT delorie DOT com" targe=
t=3D"_blank">geda-user AT delorie DOT com</a>]" <<a href=3D"mailto:geda-us=
er AT delorie DOT com" target=3D"_blank">geda-user AT delorie DOT com</a>> wrote:<br t=
ype=3D"attribution"><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0=
.8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir=3D"ltr"><div><d=
iv><div>Hi Britton,<br><br></div>I'm reading patches now, and am about =
on IRC.<br><br></div>Currently just going through branch by branch, making =
some summaries - which I can share with you here, on IRC, or off-list.<br><=
br></div>Peter<br><div><div><br></div></div></div><div class=3D"gmail_extra=
"><br><div class=3D"gmail_quote">On 3 January 2016 at 00:33, Britton Kerin =
(<a href=3D"mailto:britton DOT kerin AT gmail DOT com" target=3D"_blank">britton.kerin=
@gmail.com</a>) [via <a href=3D"mailto:geda-user AT delorie DOT com" target=3D"_bl=
ank">geda-user AT delorie DOT com</a>] <span dir=3D"ltr"><<a href=3D"mailto:ged=
a-user AT delorie DOT com" target=3D"_blank">geda-user AT delorie DOT com</a>></span> =
wrote:<br><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;bord=
er-left:1px #ccc solid;padding-left:1ex"><br><div class=3D"gmail_quote"><sp=
an>On Sat, Jan 2, 2016 at 12:53 PM, Peter Clifton (<a href=3D"mailto:peterc=
jclifton AT googlemail DOT com" target=3D"_blank">petercjclifton AT googlemail DOT com</a=
>) [via <a href=3D"mailto:geda-user AT delorie DOT com" target=3D"_blank">geda-use=
r AT delorie DOT com</a>] <span dir=3D"ltr"><<a href=3D"mailto:geda-user AT delori=
e.com" target=3D"_blank">geda-user AT delorie DOT com</a>></span> wrote:<br><bl=
ockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px #=
ccc solid;padding-left:1ex"><p dir=3D"ltr">Excellent,</p>
<p dir=3D"ltr">I think doing some interactive review / chattting on IRC wil=
l probably be a fun way to look at this. I might ask you to take a look at =
some of mine too. (If we catch Bert or DJ too by any chance, that would als=
o be cool).</p>
<p dir=3D"ltr">What timezone are you in? I'm GMT, but might not be up s=
uper early tomorrow after two busy days I've spent in London.</p></bloc=
kquote></span><div>Alaska UTC-9.=C2=A0 I'll set email to beep tomorrow<=
br></div></div>
</blockquote></div><br></div>
</blockquote></div>
</div></div></blockquote></div><br></div>
--001a113aca4e7891d0052878a7e7--
- Raw text -