"), 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

On Sun, Jan 3, 2016 at 5:12 PM, Peter Clifton (petercjclifton AT googlemail DOT com) [via geda-use= r AT delorie DOT com] <geda-user AT delorie DOT com> wrote:

I'm probably startin= g 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=20 time or other, either inadvertently, deliberately, or out of laziness! - Be= ar 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 thes= e 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 ind= ividual patches...

"//" comments are not PCB style.<= br>

Ok.=C2=A0 I'll ju= st post-process them all back to /**/ at the end of an arc

=C2=A0=

Nor is "if () = {" in the core... it is:

if (condition)
=C2=A0 {<= br>
=C2=A0=C2=A0=C2=A0 code();
=C2=A0=C2=A0=C2=A0 more_code();=
=C2=A0 }

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.

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.

=C2=A0

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.

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.

=C2=A0

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,
=

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").

Ok.

=C2=A0

=
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.

I agree that this is better for simple changes when the chance of u= nintended interactions looks small.

=C2=A0

=
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

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

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

= 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.

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.

=C2=A0

On to the specifics:

"backtr= ace"

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 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.

=C2=A0

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

= 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

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

developers - we don't ship code that crashes - right?

Nooooo.=C2=A0 Not us

=C2=A0

DJ suggested we could commit with= the code disabled, which I'd be ok with.

<= div>

Sounds good to me.

=C2=A0

"home/bkerin/debugging_markers"

I&= #39;m not keen on this, but don't have any strong objections.

Th= e main ones I had:
=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.
=C2=A0 Why max= imum 1042 markers? (Why not 1043, 666, ???)

42 is magical thanks to your noble sainted dear departed c= ompatriot Douglas Adams and means "this number was chosen arbitrarily&= quot;.

=C2=A0

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).

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.

=C2=A0

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.

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.

=C2=A0

"home/bkerin/drc_warning_for_noobs&q= uot;

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).

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.

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.

=C2=A0

"ho= me/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.=C2=A0 I like and use a number of features of C99, in particular i= n-line declarations and compound literals.
=C2=A0
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!).

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:

=C2=A0 = Vec spa_spb; =C2=A0 =C2=A0// Vector from segment point a to point b

=C2=A0 Vec spa_pt; =C2=A0 =C2=A0 // Vector from segment point a to pt

=C2=A0 Vec ptl; =C2=A0 =C2=A0 =C2=A0 =C2=A0// Projection of pt onto= seg vector (onto spa_spb)

=C2=A0 FT sm; =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0// Segment Magnitude

=C2=A0 FT pm; =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0// Projection Magnitude

=C2=A0 Point pp; =C2=A0 =C2= =A0 =C2=A0 // Projected Point

=C2=A0 FT sppm; =C2=A0 =C2=A0 =C2= =A0 =C2=A0// Magnitude spa_spb + spa_pt (Segment Plus Proj. Mag.)

=C2=A0 Point result; =C2=A0 // Result to be returned

<= div>Then a check for a degenerate argument and then this:

=C2=A0 spa_spb =3D vec_from (seg->pa, seg->pb);

=C2=A0 spa_pt =C2=A0=3D vec_from (seg->pa, pt);

=C2=A0 ptl = =C2=A0 =C2=A0 =3D vec_proj (spa_pt, spa_spb);

=C2=A0 sm =C2=A0 = =C2=A0 =C2=A0=3D vec_mag (spa_spb);

=C2=A0 pm =C2=A0 =C2=A0 =C2= =A0=3D vec_mag (ptl);

=C2=A0 pp =C2=A0 =C2=A0 =C2=A0=3D vec_sum (= seg->pa, ptl);

=C2=A0 sppm =C2=A0 =C2=A0=3D 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 ma= ke it harder to read.

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.

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.

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.
<= /div>

When it came up you were the only nom= inee, so it's pretty safe to say you get to decide this stuff.

=C2=A0

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??)

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.

=C2=A0

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>
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.

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.

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).

=C2=A0

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..).
=
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!).

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.

=
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.=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.

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

=C2=A0

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.
=

=C2=A0

= 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.

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.

=C2=A0

<= 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">

I can help if required, and could - f= or example, put together an alternative stack-up of patches by way of examp= le.

Re-ordering = and/or refactoring those commits really sounds pretty excruciating to me.= =C2=A0 How about we do this:

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).

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

=C2=A0 =C2=A0 is in s= ome ways incompatible with your retro-cleanish-room approach, but you can j= udge for yourself.

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.

Britton

--089e0122f0883e67c605287e6af6--