Mail Archives: geda-user/2016/01/04/04:05:47
--089e0122f0883e67c605287e6af6
Content-Type: text/plain; charset=UTF-8
On Sun, Jan 3, 2016 at 5:12 PM, Peter Clifton (petercjclifton AT googlemail DOT com)
[via geda-user AT delorie DOT com] <geda-user AT delorie DOT com> wrote:
> 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.
>
Ok. I'll just post-process them all back to /**/ at the end of an arc
> 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.
>
Ok. I noticed that it was variable already and assumed it wasn't a big
worry. I'd urge relaxation on this sort of issue, because I think all that
really matters is local consistency, but it wasn't my intention to force
the issue and I'm happy to go along.
> 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.
>
Fair enough. I change the spacing sometimes when I think it will reduce
the chance that I'll make actual errors. It sounds mundane but putting
things in columns reduces errors for me. Letting individual programmers
use their own style might also result in fewer actual errors.
> 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").
>
Ok.
> 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.
>
I agree that this is better for simple changes when the chance of
unintended interactions looks small.
> 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
>
Ug sorry. I dislike deleting them when using rebase because it makes it
harder to go back and recover your original branch if it interacts badly
with something else, in a bad case you don't even remember where you
originally branched from and end up having to apply the whole sequence
again and again. But clearly they should be marked somehow to avoid that
sort of confusion, maybe moved to home/whoever/already_merged or something
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.
>
Well I actually use it constantly during DRC. It's weird but good, saves a
point on a frequent activity. Marcus thought having it on double-click was
probably ok, but I guess it could be turned off by default.
> 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).
>
I could make so it just isn't there on win32 of course. I can't show that
*anything* I've done will definitely build and run on Win32. backtrace()
definitely won't work there. I thought win32 was broken anyway.
> 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
>
Not me, I quit using gdb about the time every project I worked on started
using 4 or so languages. Who wants to remember 4*2=8 ways of saying
printf? I do valgrind everything though
I wanted backtrace because rtree.c occasionally assert fails (in master and
IIRC release as well). I can't reproduce it reliably and doubt it's really
a bug but the next time it happens I'd like to have a better shot at
sorting it out. Unless you like to run under debugger always it could
still be useful for tracking down bugs like this even for people who like
debuggers
developers - we don't ship code that crashes - right?
>
Nooooo. Not us
> DJ suggested we could commit with the code disabled, which I'd be ok with.
>
Sounds good to me.
> "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, ???)
>
42 is magical thanks to your noble sainted dear departed compatriot Douglas
Adams and means "this number was chosen arbitrarily".
> 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 won't be a tragedy if you have to blow it away again, and I'd say that
should go for debugging code in general. It's supposed to make things
easier and if the best contribution it can make the next time it's relevant
at all is to go away, that's what should happen. Pinging the original
author if they're still around is a bonus.
> 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.
>
My philosophy for which debugging code should get committed seems different
than yours. You seem to favor having none of it in. I like to put in
things that seem particularly likely to be generally desirable (e.g.
backtraces are available in most languages now; it's often convenient to be
able to check the result of a geometrical calculation visually). DJ's
approach (#ifdef DEBUG it or so) works fine for me.
> "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.
>
Perhaps. They are quite different blobs of code so combining them in the
GUI might count as dishonesty about the implementation, which often leads
to trouble and confusion, though I can't see how it would in this case. I
don't love the warning either, but I do think it's undeniably better than
the existing situation (in which the disappearance of the last violation
strongly signals correctness, when that's not at all guaranteed, and
possibly for an almost identical reason to the ones DRC noticably does
handle e.g. actual short after near shorts). It could be put in until
someone finds the time for a better fix.
> "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.
>
Yes rebase/cherry-pick is fine for that sort of thing. I like and use a
number of features of C99, in particular in-line declarations and compound
literals.
> 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!).
>
I like declare-where-used and consider it good style, mainly because it
reduces redundancy. For example geometry.c currently has this:
Vec spa_spb; // Vector from segment point a to point b
Vec spa_pt; // Vector from segment point a to pt
Vec ptl; // Projection of pt onto seg vector (onto spa_spb)
FT sm; // Segment Magnitude
FT pm; // Projection Magnitude
Point pp; // Projected Point
FT sppm; // Magnitude spa_spb + spa_pt (Segment Plus Proj. Mag.)
Point result; // Result to be returned
Then a check for a degenerate argument and then this:
spa_spb = vec_from (seg->pa, seg->pb);
spa_pt = vec_from (seg->pa, pt);
ptl = vec_proj (spa_pt, spa_spb);
sm = vec_mag (spa_spb);
pm = vec_mag (ptl);
pp = vec_sum (seg->pa, ptl);
sppm = vec_mag (vec_sum (spa_spb, ptl));
It just gets twice as long, repeats the names, and separates the types from
the values, all of which make it harder to read.
Regarding the length argument, if not using declare-where-used cured people
of the tendency to write too-long functions it would be worth it, but it
doesn't.
That said I have to admit I sometimes appreciate having a catalog of
automatic variables all in one place. It's particularly useful in
otherwise undocumented functions as a sort of enforced summary. But given
a choice I'd rather have a summary comment. It's another issue where it
seems best to me to trust the judgement of the individual developer and let
them use the style they prefer.
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.
>
When it came up you were the only nominee, so it's pretty safe to say you
get to decide this stuff.
> 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??)
>
geometry_module was off the wrong commit and geometry_module2 is the one to
look at, though everything in it is also in fix_drc_violation_locations off
which now branches warn_about_invalid_flags. I've emailed the list about
the situation but it's still a confusing mess, sorry. I didn't mean to get
so many branches going.
> 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.
>
Yep. It would be simplest to just fix them on the end of the big branch
and ignore their independent branches (which I only maintained for clarity).
> 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!).
>
I have zero desire to be uncooperative, but I don't really want to do
this. I'll go through and mark any non-build commits if you want. I
suppose they could just be eliminated by combining with neighbors, but I
normally never do that, don't know how, and doubt that doing so would would
yield a genuinely more useful history. There's no chance of them all
working the way I do things, because I instrument or assert0 call points
and the like to ensure they get code coverage testing at least, and some of
that instrumentation inevitably persists for many commits.
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
>
Yes, but *any *re-read of own code usually yields some places that things
could be improved. I do habitually do that before every commit. I forgot
to do the whole-diff review so there was some junk left that DJ caught, but
not much, for this reason. You also have to compare the effectiveness of
sanitizing the history to that of other activities, test writing in
particular.
If it's any reassurance I'm aware of clean-room design, sequences of
provable transformations, etc. and try for something like that per patch.
I've just never gone as far as rewriting history to achieve it
retroactively. Maybe it does yield the same results as the
ultra-fastidious up-front version, but I'm a little skeptical.
> 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.
>
If I had to choose I'd rather write tests. I don't know how the pcb
framework for that works but have the impression there isn't much done on
it yet. The Arc code at least I'm 100% confident is much less buggy than
before. Honestly I don't think the old stuff got tested much at all before
going in. The line intersection rewrite deserves the closest scrutiny
since everything uses it and the original wasn't broken, so it might be
worth identifying and possibly grouping those commits for careful review.
> I can help if required, and could - for example, put together an
> alternative stack-up of patches by way of example.
>
Re-ordering and/or refactoring those commits really sounds pretty
excruciating to me. How about we do this:
1. Let me make the last few changes i have in mind on
warn_about_invalid_flags (itself a branch off fix_drc_violation_locations).
2. You take a look at it and consider what you think would be required. I
mentioned how I instrument above. I suspect that my overall approach
is in some ways incompatible with your retro-cleanish-room approach,
but you can judge for yourself.
3. We then agree what approach is best to get to required confidence with
the more risky changes, be that tests or something else.
Britton
--089e0122f0883e67c605287e6af6
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
<div dir=3D"ltr"><br><div class=3D"gmail_extra"><br><div class=3D"gmail_quo=
te">On Sun, Jan 3, 2016 at 5:12 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:0px 0px 0px 0.8ex;border-lef=
t-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padd=
ing-left:1ex"><div dir=3D"ltr"><div><div><div><div>I'm probably startin=
g to run out of energy for tonight, having got chatting on IRC...<br><br><d=
iv>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=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></div></div></blockquote><div><br></div><div>Ok.=C2=A0 I'll ju=
st post-process them all back to /**/ at the end of an arc</div><div>=C2=A0=
</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;b=
order-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:s=
olid;padding-left:1ex"><div dir=3D"ltr"><div><div></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 HI=
D, 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 respe=
ct 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 pro=
bably a reasonable compromise though... if it is a huge imposition to fix a=
reas where you've written significant amounts of new code.<br></div></d=
iv></blockquote><div><br></div><div>Ok.=C2=A0 I noticed that it was variabl=
e already and assumed it wasn't a big worry.=C2=A0 I'd urge relaxat=
ion on this sort of issue, because I think all that really matters is local=
consistency, but it wasn't my intention to force the issue and I'm=
happy to go along.</div><div>=C2=A0</div><blockquote class=3D"gmail_quote"=
style=3D"margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:=
rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir=3D"ltr"=
><div>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 en=
counter them, just do it as a separate commit. (Having stgit helps no end h=
ere). 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 w=
ork around certain areas of code.<br></div></div></blockquote><div><br></di=
v><div>Fair enough.=C2=A0 I change the spacing sometimes when I think it wi=
ll reduce the chance that I'll make actual errors.=C2=A0 It sounds mund=
ane but putting things in columns reduces errors for me.=C2=A0 Letting indi=
vidual programmers use their own style might also result in fewer actual er=
rors.</div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"marg=
in:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,20=
4);border-left-style:solid;padding-left:1ex"><div dir=3D"ltr"><div>FWIW, My=
general rule of thumb, is I'll let myself make white-space changes wit=
hin a few lines of a real code-change, if it is in the same function,</div>=
</div></blockquote><blockquote class=3D"gmail_quote" style=3D"margin:0px 0p=
x 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border=
-left-style:solid;padding-left:1ex"><div dir=3D"ltr"><div> and makes things=
look less ugly overall. (Obviously, fixing trailing white-space on lines w=
ith real code-changes is fine too). Anything more invasive, ought to be a s=
eparate patch (although I'm sure you'd be able to find commits wher=
e I've broken this "rule").<br></div></div></blockquote><div>=
<br></div><div>Ok.</div><div>=C2=A0</div><blockquote class=3D"gmail_quote" =
style=3D"margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:r=
gb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir=3D"ltr">=
<div>When we get to committing / merging, I'd prefer to see your simple=
r changes fixed (if required), and committed (cherry-picked?) on top of mas=
ter - but its not super important.<br></div></div></blockquote><div><br></d=
iv><div>I agree that this is better for simple changes when the chance of u=
nintended interactions looks small.</div><div>=C2=A0</div><blockquote class=
=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left-width:1px;bo=
rder-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">=
<div dir=3D"ltr"><div>Do you have any thoughts on deleting branches once me=
rged? (I dumped a list of branches, and spent a long time reviewing=C2=A0</=
div></div></blockquote><blockquote class=3D"gmail_quote" style=3D"margin:0p=
x 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);bo=
rder-left-style:solid;padding-left:1ex"><div dir=3D"ltr"><div>home/bkerin/d=
rc_fixes before I realised it was already merged!) I had many comments on t=
hat one (some of which I now see you addressed</div></div></blockquote><div=
><br></div><div>Ug sorry.=C2=A0 I dislike deleting them when using rebase b=
ecause it makes it harder to go back and recover your original branch if it=
interacts badly with something else, in a bad case you don't even reme=
mber where you originally branched from and end up having to apply the whol=
e sequence again and again.=C2=A0 But clearly they should be marked somehow=
to avoid that sort of confusion, maybe moved to home/whoever/already_merge=
d or something</div><div><br></div><blockquote class=3D"gmail_quote" style=
=3D"margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(20=
4,204,204);border-left-style:solid;padding-left:1ex"><div dir=3D"ltr"><div>=
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 som=
ething which needs discussing now though.<br></div></div></blockquote><div>=
<br></div><div>Well I actually use it constantly during DRC.=C2=A0 It's=
weird but good, saves a point on a frequent activity.=C2=A0 Marcus thought=
having it on double-click was probably ok, but I guess it could be turned =
off by default.</div><div>=C2=A0</div><blockquote class=3D"gmail_quote" sty=
le=3D"margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(=
204,204,204);border-left-style:solid;padding-left:1ex"><div dir=3D"ltr"><di=
v>On to the specifics:<br></div><div><br></div><div></div><div>"backtr=
ace"<br><br>Big NAK on merging unless you can show it builds and runs =
on Win32. (Or can be committed, but disabled by default).<br></div></div></=
blockquote><div><br></div><div>I could make so it just isn't there on w=
in32 of course.=C2=A0 I can't show that *anything* I've done will d=
efinitely build and run on Win32. =C2=A0backtrace() definitely won't wo=
rk there.=C2=A0 I thought win32 was broken anyway.</div><div>=C2=A0</div><b=
lockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-le=
ft-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;pad=
ding-left:1ex"><div dir=3D"ltr"><div>This is not the kind of code I like ca=
rrying - developers debug under gdb and valgrind (so it is of less use to t=
hem), and since we're good</div></div></blockquote><div><br></div><div>=
Not me, I quit using gdb about the time every project I worked on started u=
sing 4 or so languages.=C2=A0 Who wants to remember 4*2=3D8 ways of saying =
printf?=C2=A0 I do valgrind everything though</div><div><br></div><div>I wa=
nted backtrace because rtree.c occasionally assert fails (in master and IIR=
C release as well).=C2=A0 I can't reproduce it reliably and doubt it=
9;s really a bug but the next time it happens I'd like to have a better=
shot at sorting it out.=C2=A0 Unless you like to run under debugger always=
it could still be useful for tracking down bugs like this even for people =
who like debuggers</div><div><br></div><blockquote class=3D"gmail_quote" st=
yle=3D"margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb=
(204,204,204);border-left-style:solid;padding-left:1ex"><div dir=3D"ltr"><d=
iv> developers - we don't ship code that crashes - right?<br></div></di=
v></blockquote><div><br></div><div>Nooooo.=C2=A0 Not us</div><div>=C2=A0</d=
iv><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;bord=
er-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:soli=
d;padding-left:1ex"><div dir=3D"ltr"><div>DJ suggested we could commit with=
the code disabled, which I'd be ok with.<br></div></div></blockquote><=
div><br></div><div>Sounds good to me.</div><div>=C2=A0</div><blockquote cla=
ss=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left-width:1px;=
border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex=
"><div dir=3D"ltr"><div>"home/bkerin/debugging_markers"<br><br>I&=
#39;m not keen on this, but don't have any strong objections.<br><br>Th=
e main ones I had:<br></div>=C2=A0 Avoid use of assert(), especially to che=
ck bounds when "n" markers are being added - this is ugly - you j=
ust KNOW it will fire, and cause someone a headache.<br><div>=C2=A0 Why max=
imum 1042 markers? (Why not 1043, 666, ???)<br></div></div></blockquote><di=
v><br></div><div>42 is magical thanks to your noble sainted dear departed c=
ompatriot Douglas Adams and means "this number was chosen arbitrarily&=
quot;.<br></div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D=
"margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,2=
04,204);border-left-style:solid;padding-left:1ex"><div dir=3D"ltr"><div>I m=
ay be a tiny bit biased, as I suspect this one will cause me merge pain whe=
n I come to rebase my own monster pile of patches on top of it. (I've d=
one a fair bit of re-factoring of the HID / drawing interface).<br></div></=
div></blockquote><div><br></div><div>It won't be a tragedy if you have =
to blow it away again, and I'd say that should go for debugging code in=
general.=C2=A0 It's supposed to make things easier and if the best con=
tribution it can make the next time it's relevant at all is to go away,=
that's what should happen.=C2=A0 Pinging the original author if they&#=
39;re still around is a bonus.</div><div>=C2=A0</div><blockquote class=3D"g=
mail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left-width:1px;border-=
left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div =
dir=3D"ltr"><div>It also strikes me as the kind of thing I'd end up wan=
ting to pull out (perhaps years down the line)... once we've finished u=
sing 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 drownin=
g in dead-code if I committed all the ones I ever used whilst developing ne=
w features.<br></div></div></blockquote><div><br></div><div>My philosophy f=
or which debugging code should get committed seems different than yours.=C2=
=A0 You seem to favor having none of it in.=C2=A0 I like to put in things t=
hat seem particularly likely to be generally desirable (e.g. backtraces are=
available in most languages now; it's often convenient to be able to c=
heck the result of a geometrical calculation visually).=C2=A0 DJ's appr=
oach (#ifdef DEBUG it or so) works fine for me.</div><div>=C2=A0</div><bloc=
kquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left-=
width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;paddin=
g-left:1ex"><div dir=3D"ltr"><div>"home/bkerin/drc_warning_for_noobs&q=
uot;<br><br>This one offends my sense of OCD, to leave a DRC warning on eve=
ry design, which cannot be fixed! (But technically, it is ready for being c=
herry-picked).<br><br></div><div>IMO, a better fix - if we care, is to actu=
ally run the rats-nest check underlying the "O" command (refactor=
if needed), to see whether any shorts or missing nets remain. Add DRC warn=
ing 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.<br></div></div></blo=
ckquote><div><br></div><div>Perhaps.=C2=A0 They are quite different blobs o=
f code so combining them in the GUI might count as dishonesty about the imp=
lementation, which often leads to trouble and confusion, though I can't=
see how it would in this case.=C2=A0 I don't love the warning either, =
but I do think it's undeniably better than the existing situation (in w=
hich the disappearance of the last violation strongly signals correctness, =
when that's not at all guaranteed, and possibly for an almost identical=
reason to the ones DRC noticably does handle e.g. actual short after near =
shorts).=C2=A0 It could be put in until someone finds the time for a better=
fix.</div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"marg=
in:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,20=
4);border-left-style:solid;padding-left:1ex"><div dir=3D"ltr"><div>"ho=
me/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-picked into master, not merged.<br></div></div></b=
lockquote><div><br></div><div>Yes rebase/cherry-pick is fine for that sort =
of thing.=C2=A0 I like and use a number of features of C99, in particular i=
n-line declarations and compound literals.</div><div>=C2=A0</div><blockquot=
e class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left-width=
:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-lef=
t:1ex"><div dir=3D"ltr"><div>NAK if the reason is to enable laxer coding st=
yle, without a general decision that we want to adopt such a style. (That i=
ncludes "//" comments, and I particularly dislike defining variab=
les mid-function scope - if it helps, then your functions are too long!).<b=
r></div></div></blockquote><div><br></div><div>I like declare-where-used an=
d consider it good style, mainly because it reduces redundancy.=C2=A0 For e=
xample geometry.c currently has this:</div><div><br></div><div><div>=C2=A0 =
Vec spa_spb; =C2=A0 =C2=A0// Vector from segment point a to point b</div><d=
iv>=C2=A0 Vec spa_pt; =C2=A0 =C2=A0 // Vector from segment point a to pt</d=
iv><div>=C2=A0 Vec ptl; =C2=A0 =C2=A0 =C2=A0 =C2=A0// Projection of pt onto=
seg vector (onto spa_spb)</div><div>=C2=A0 FT sm; =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0// Segment Magnitude</div><div>=C2=A0 FT pm; =C2=A0 =C2=A0 =C2=A0=
=C2=A0 =C2=A0// Projection Magnitude</div><div>=C2=A0 Point pp; =C2=A0 =C2=
=A0 =C2=A0 // Projected Point</div><div>=C2=A0 FT sppm; =C2=A0 =C2=A0 =C2=
=A0 =C2=A0// Magnitude spa_spb + spa_pt (Segment Plus Proj. Mag.)</div><div=
>=C2=A0 Point result; =C2=A0 // Result to be returned</div><div><br></div><=
div>Then a check for a degenerate argument and then this:</div><div><br></d=
iv><div><div>=C2=A0 spa_spb =3D vec_from (seg->pa, seg->pb);</div><di=
v>=C2=A0 spa_pt =C2=A0=3D vec_from (seg->pa, pt);</div><div>=C2=A0 ptl =
=C2=A0 =C2=A0 =3D vec_proj (spa_pt, spa_spb);</div><div>=C2=A0 sm =C2=A0 =
=C2=A0 =C2=A0=3D vec_mag (spa_spb);</div><div>=C2=A0 pm =C2=A0 =C2=A0 =C2=
=A0=3D vec_mag (ptl);</div><div>=C2=A0 pp =C2=A0 =C2=A0 =C2=A0=3D vec_sum (=
seg->pa, ptl);</div><div>=C2=A0 sppm =C2=A0 =C2=A0=3D vec_mag (vec_sum (=
spa_spb, ptl));</div></div><div><br></div><div>It just gets twice as long, =
repeats the names, and separates the types from the values, all of which ma=
ke it harder to read.</div></div><div><br></div><div>Regarding the length a=
rgument, if not using declare-where-used cured people of the tendency to wr=
ite too-long functions it would be worth it, but it doesn't.</div><div>=
<br></div><div>That said I have to admit I sometimes appreciate having a ca=
talog of automatic variables all in one place.=C2=A0 It's particularly =
useful in otherwise undocumented functions as a sort of enforced summary.=
=C2=A0 But given a choice I'd rather have a summary comment.=C2=A0 It&#=
39;s another issue where it seems best to me to trust the judgement of the =
individual developer and let them use the style they prefer.</div><div><br>=
</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;b=
order-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:s=
olid;padding-left:1ex"><div dir=3D"ltr"><div>NB: By "general discussio=
n", I mean - we pick 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></blockquote><div><br></div><div>When it came up you were the only nom=
inee, so it's pretty safe to say you get to decide this stuff.</div><di=
v>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px=
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left=
-style:solid;padding-left:1ex"><div dir=3D"ltr"><div>I have not yet looked =
in depth at any of the code changes the big complex branches, "geometr=
y_module", "geometry_module2" and "fix_drc_violation_lo=
cations". (Is one of the "geometry_module.." branches now re=
dundant??)<br></div></div></blockquote><div><br></div><div>geometry_module =
was off the wrong commit and geometry_module2 is the one to look at, though=
everything in it is also in fix_drc_violation_locations off which now bran=
ches warn_about_invalid_flags.=C2=A0 I've emailed the list about the si=
tuation but it's still a confusing mess, sorry.=C2=A0 I didn't mean=
to get so many branches going.</div><div>=C2=A0</div><blockquote class=3D"=
gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left-width:1px;border=
-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div=
dir=3D"ltr"><div>If these do what I think they do - then I'm looking f=
orward to seeing the results. (Improving the DRC output is a really importa=
nt task - one which has obviously been needed since back when we added the =
abstracted DRC with the better ability to preview the violation location).<=
br><br>Skimming the commits in these branches, I think they might benefit f=
rom 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 str=
ucture, and churn.<br><br></div><div>It appears that many of the patches si=
t 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.<br></div></=
div></blockquote><div><br></div><div style=3D"">Yep.=C2=A0 It would be simp=
lest to just fix them on the end of the big branch and ignore their indepen=
dent branches (which I only maintained for clarity).</div><div>=C2=A0</div>=
<blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-=
left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;p=
adding-left:1ex"><div dir=3D"ltr"><div>Looking at the commit comments, it a=
ppears 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 ful=
ly functional. (This really helps with bisecting to find errors etc..).<br>=
<br></div><div>If I were you, I would pull that entire branch into stgit (&=
quot;stg uncommit -n <number of patches>"), and start moving cod=
e about, re-factoring patches, until the deltas for each commit are clean, =
all commits build (and ideally work!).<br></div></div></blockquote><div><br=
></div><div style=3D"">I have zero desire to be uncooperative, but I don=
9;t really want to do this.=C2=A0 I'll go through and mark any non-buil=
d commits if you want.=C2=A0 I suppose they could just be eliminated by com=
bining with neighbors, but I normally never do that, don't know how, an=
d doubt that doing so would would yield a genuinely more useful history.=C2=
=A0 There's no chance of them all working the way I do things, because =
I instrument or assert0 call points and the like to ensure they get code co=
verage testing at least, and some of that instrumentation inevitably persis=
ts for many commits.</div><div><br></div><blockquote class=3D"gmail_quote" =
style=3D"margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:r=
gb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir=3D"ltr">=
<div>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</d=
iv></div></blockquote><div><br></div><div style=3D"">Yes, but *any *re-read=
of own code usually yields some places that things could be improved.=C2=
=A0 I do habitually do that before every commit.=C2=A0 I forgot to do the w=
hole-diff review so there was some junk left that DJ caught, but not much, =
for this reason.=C2=A0 You also have to compare the effectiveness of saniti=
zing the history to that of other activities, test writing in particular.</=
div><div><br></div><div style=3D"">If it's any reassurance I'm awar=
e of clean-room design, sequences of provable transformations, etc. and try=
for something like that per patch.=C2=A0 I've just never gone as far a=
s rewriting history to achieve it retroactively.=C2=A0 Maybe it does yield =
the same results as the ultra-fastidious up-front version, but I'm a li=
ttle skeptical.=C2=A0</div><div>=C2=A0</div><blockquote class=3D"gmail_quot=
e" style=3D"margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-colo=
r:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir=3D"lt=
r"><div> key difference between working with SCM systems like git (which le=
t 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.</div>=
</div></blockquote><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=
=3D"margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(20=
4,204,204);border-left-style:solid;padding-left:1ex"><div dir=3D"ltr"><div>=
I'm still keen to do an actual code-review of the changes in these bigg=
er branches, but also wanted to get your thoughts on willingness to try out=
stgit, and/or shuffle patches around.<br></div></div></blockquote><div><br=
></div><div style=3D"">If I had to choose I'd rather write tests.=C2=A0=
I don't know how the pcb framework for that works but have the impress=
ion there isn't much done on it yet.=C2=A0 The Arc code at least I'=
m 100% confident is much less buggy than before.=C2=A0 Honestly I don't=
think the old stuff got tested much at all before going in.=C2=A0 The line=
intersection rewrite deserves the closest scrutiny since everything uses i=
t and the original wasn't broken, so it might be worth identifying and =
possibly grouping those commits for careful review.</div><div>=C2=A0</div><=
blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l=
eft-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;pa=
dding-left:1ex"><div dir=3D"ltr"><div>I can help if required, and could - f=
or example, put together an alternative stack-up of patches by way of examp=
le.<br></div></div></blockquote><div><br></div><div style=3D"">Re-ordering =
and/or refactoring those commits really sounds pretty excruciating to me.=
=C2=A0 How about we do this:</div><div style=3D""><br></div><div style=3D""=
>1. Let me make the last few changes i have in mind on warn_about_invalid_f=
lags (itself a branch off fix_drc_violation_locations).</div><div style=3D"=
"><br></div><div style=3D"">2. You take a look at it and consider what you =
think would be required.=C2=A0 I mentioned how I instrument above.=C2=A0 I =
suspect that my overall approach</div><div style=3D"">=C2=A0 =C2=A0 is in s=
ome ways incompatible with your retro-cleanish-room approach, but you can j=
udge for yourself.</div><div><br></div><div style=3D"">3. We then agree wha=
t approach is best to get to required confidence with the more risky change=
s, be that tests or something else.</div><div style=3D""><br></div><div sty=
le=3D"">Britton</div><div style=3D""><br></div></div></div></div>
--089e0122f0883e67c605287e6af6--
- Raw text -