Mail Archives: geda-user/2016/01/04/07:08:20
--001a11369ad65a560a052880f730
Content-Type: text/plain; charset=UTF-8
Ok, sounds like a positive way to proceed, and the DEBUG define looks like
a clean way to hide the backtrace code from builds which won't need it or
won't work with it.
I'm alarmed to hear that your seeing the rtree code assert. I guess you are
running with DEBUG on, and that perhaps we've missed something that crept
in at some point. This could be a tricky one, and I'd be interested to see
any data you had on where it is asserting?
I can't entirely recall if it was pcb or gschem where the arc code was
already re written once during my involvement with the project (not by me).
In the case I'm thinking of, the author told me they had written a
monte-carlo test to throw arbitrary data at the code and test for bugs.
This kind of thing might be cool to try if you were keen to write test
cases.
With regards rewriting geometry functions, I'd be interested in a why - if
they already worked. The old code looking ugly would be a fine reason, just
be extra sure the replacements will be well behaved ;)
At the end of the day, if these changes don't cause any problems, we could
just push / merge as is. Hopefully we won't need to be bisecting across
them. Keeping a clean patch series is more about review than anything else
for me, and I do understand there are risks and tradeoffs involved with
rebasing and juggling patch series. (I backup a branch, perhaps by tagging
its HEAD before doing a big rebase).
I'll ask my kernel maintainer colleague how they deal with big branches /
patch series, for some comparison. I think there - they only merge
completed changes, and all the deltas are very clean. The projects aren't
exactly comparable of course - so we might decide to be different here.
If I catch you on IRC at some point, I'll probably ask you to walk me
through a quick high level summary of the big branch. That, and a skim of
the diffs is probably the most "help" I can be at the moment, and will
hopefully give you an extra thumbs up to merge in.
Perhaps I can ask you to do the same with me, on some of my branches after
that? I at least need to rebase against HEAD first :).
Peter
On 4 Jan 2016 09:07, "Britton Kerin (britton DOT kerin AT gmail DOT com) [via
geda-user AT delorie DOT com]" <geda-user AT delorie DOT com> wrote:
>
>
> 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
>
>
--001a11369ad65a560a052880f730
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
<p dir=3D"ltr">Ok, sounds like a positive way to proceed, and the DEBUG def=
ine looks like a clean way to hide the backtrace code from builds which won=
't need it or won't work with it.</p>
<p dir=3D"ltr">I'm alarmed to hear that your seeing the rtree code asse=
rt. I guess you are running with DEBUG on, and that perhaps we've misse=
d something that crept in at some point. This could be a tricky one, and I&=
#39;d be interested to see any data you had on where it is asserting?</p>
<p dir=3D"ltr">I can't entirely recall if it was pcb or gschem where th=
e arc code was already re written once during my involvement with the proje=
ct (not by me). In the case I'm thinking of, the author told me they ha=
d written a monte-carlo test to throw arbitrary data at the code and test f=
or bugs. This kind of thing might be cool to try if you were keen to write =
test cases.</p>
<p dir=3D"ltr">With regards rewriting geometry functions, I'd be intere=
sted in a why - if they already worked. The old code looking ugly would be =
a fine reason, just be extra sure the replacements will be well behaved ;)<=
/p>
<p dir=3D"ltr">At the end of the day, if these changes don't cause any =
problems, we could just push / merge as is. Hopefully we won't need to =
be bisecting across them. Keeping a clean patch series is more about review=
than anything else for me,=C2=A0 and I do understand there are risks and t=
radeoffs involved with rebasing and juggling patch series. (I backup a bran=
ch, perhaps by tagging its HEAD before doing a big rebase).</p>
<p dir=3D"ltr">I'll ask my kernel maintainer colleague how they deal wi=
th big branches / patch series, for some comparison. I think there - they o=
nly merge completed changes, and all the deltas are very clean. The project=
s aren't exactly comparable of course - so we might decide to be differ=
ent here.</p>
<p dir=3D"ltr">If I catch you on IRC at some point, I'll probably ask y=
ou to walk me through a quick high level summary of the big branch. That, a=
nd a skim of the diffs is probably the most "help" I can be at th=
e moment, and will hopefully give you an extra thumbs up to merge in.</p>
<p dir=3D"ltr">Perhaps I can ask you to do the same with me, on some of my =
branches after that? I at least need to rebase against HEAD first :).</p>
<p dir=3D"ltr">Peter<br><br><br></p>
<div class=3D"gmail_quote">On 4 Jan 2016 09:07, "Britton Kerin (<a hre=
f=3D"mailto:britton DOT kerin AT gmail DOT com">britton DOT kerin AT gmail DOT com</a>) [via <a h=
ref=3D"mailto:geda-user AT delorie DOT com">geda-user AT delorie DOT com</a>]" <<=
a href=3D"mailto:geda-user AT delorie DOT com">geda-user AT delorie DOT com</a>> wrote=
:<br type=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"><=
br><div class=3D"gmail_extra"><br><div class=3D"gmail_quote">On Sun, Jan 3,=
2016 at 5:12 PM, Peter Clifton (<a href=3D"mailto:petercjclifton AT googlemai=
l.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-user AT delorie DOT com</a>]=
<span dir=3D"ltr"><<a href=3D"mailto:geda-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: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><div><div><div>I'm probably starting to run out of en=
ergy for tonight, having got chatting on IRC...<br><br><div>Before I get st=
arted, I'm probably going to pick out some areas, and quote project &qu=
ot;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>Yep.=C2=A0 It would be simplest to jus=
t fix them on the end of the big branch and ignore their independent branch=
es (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;padding-left=
:1ex"><div dir=3D"ltr"><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 pushing code into master). I would =
expect that after each commit, the code should build, AND be fully function=
al. (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 ("stg un=
commit -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!).<br></div></div></blockquote><div><br></div><div=
>I have zero desire to be uncooperative, but I don't really want to do =
this.=C2=A0 I'll go through and mark any non-build commits if you want.=
=C2=A0 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.=C2=A0 There's no cha=
nce of them all working the way I do things, because I instrument or assert=
0 call points and the like to ensure they get code coverage testing at leas=
t, and some of that instrumentation inevitably persists for many commits.</=
div><div><br></div><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>This strategy can=
be time-consuming (look how long many of my patches have sat), but I reall=
y believe it yields better code changes. This is a</div></div></blockquote>=
<div><br></div><div>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 whole-diff review so there was some =
junk left that DJ caught, but not much, for this reason.=C2=A0 You also hav=
e to compare the effectiveness of sanitizing the history to that of other a=
ctivities, test writing in particular.</div><div><br></div><div>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.=C2=A0 I=
9;ve just never gone as far as rewriting history to achieve it retroactivel=
y.=C2=A0 Maybe it does yield the same results as the ultra-fastidious up-fr=
ont version, but I'm a little skeptical.=C2=A0</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> key difference between working with S=
CM systems like git (which let you prepare and finesse code changes), as co=
mpared to the bad-old-days of CVS, where developers would hack away on chan=
ges in a time-linear fashion - since managing branches and patch series was=
hard with those tools.</div></div></blockquote><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>I'm still keen to do an actual code-review=
of the changes in these bigger branches, but also wanted to get your thoug=
hts on willingness to try out stgit, and/or shuffle patches around.<br></di=
v></div></blockquote><div><br></div><div>If I had to choose I'd rather =
write tests.=C2=A0 I don't know how the pcb framework for that works bu=
t have the impression there isn't much done on it yet.=C2=A0 The Arc co=
de at least I'm 100% confident is much less buggy than before.=C2=A0 Ho=
nestly 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 it and the original wasn't broken, so it might be wort=
h 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-left-width:1px;border-left-color:rgb(204,204,204);border-l=
eft-style:solid;padding-left:1ex"><div dir=3D"ltr"><div>I can help if requi=
red, and could - for example, put together an alternative stack-up of patch=
es by way of example.<br></div></div></blockquote><div><br></div><div>Re-or=
dering and/or refactoring those commits really sounds pretty excruciating t=
o me.=C2=A0 How about we do this:</div><div><br></div><div>1. Let me make t=
he last few changes i have in mind on warn_about_invalid_flags (itself a br=
anch off fix_drc_violation_locations).</div><div><br></div><div>2. You take=
a look at it and consider what you think would be required.=C2=A0 I mentio=
ned how I instrument above.=C2=A0 I suspect that my overall approach</div><=
div>=C2=A0 =C2=A0 is in some ways incompatible with your retro-cleanish-roo=
m approach, but you can judge for yourself.</div><div><br></div><div>3. We =
then agree what approach is best to get to required confidence with the mor=
e risky changes, be that tests or something else.</div><div><br></div><div>=
Britton</div><div><br></div></div></div></div>
</blockquote></div>
--001a11369ad65a560a052880f730--
- Raw text -