X-Authentication-Warning: delorie.com: mail set sender to geda-user-bounces using -f X-Recipient: geda-user AT delorie DOT com X-Original-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlemail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; bh=BiOcoGb9M8kuZhG0pEpR6qBZec2wuGv71NE/JEU/XH0=; b=aAyl2yty6qNuQqcikv7YLJFg1CSkeGe5mgyYnxSraVzgsorkJcCIY0meygYD2z8ARK avTHyJpQ91zO6uq0m9Y63gGTMu7U7zj9K5e6VtvjZ9sAtKZpwkcWPZR6Mm4H29fzwbYT kfBaDTd9KgyvorxNqCXArFRK6FWny7eQUx9kF1cHmrFpBNLms+xr7WjpryNMMFApxUkl 41BVd5+bs5NHe63ou6wKuJXU5ZrJqe5PSXGjD/aa7zbBmsZTAJJjsgztDR5oK0XdeW2Y 50uXMUCFGtsDdnss00R5SEpE25Hp2YoA5C8Ng37bwrtRgATnw4KwWwoGmcLNoMFok6N6 IZ1A== MIME-Version: 1.0 X-Received: by 10.202.213.78 with SMTP id m75mr48493233oig.56.1451873554435; Sun, 03 Jan 2016 18:12:34 -0800 (PST) In-Reply-To: References: Date: Mon, 4 Jan 2016 02:12:34 +0000 Message-ID: Subject: Re: [geda-user] Merging stuff. How to make it happen From: "Peter Clifton (petercjclifton AT googlemail DOT com) [via geda-user AT delorie DOT com]" To: gEDA User Mailing List Content-Type: multipart/alternative; boundary=001a113aca4e7891d0052878a7e7 Reply-To: geda-user AT delorie DOT com Errors-To: nobody AT delorie DOT com X-Mailing-List: geda-user AT delorie DOT com X-Unsubscribes-To: listserv AT delorie DOT com Precedence: bulk --001a113aca4e7891d0052878a7e7 Content-Type: text/plain; charset=UTF-8 I'm probably starting to run out of energy for tonight, having got chatting on IRC... Before I get started, I'm probably going to pick out some areas, and quote project "rules" which I'm sure I've broken myself at one time or other, either inadvertently, deliberately, or out of laziness! - Bear that in mind if you think I'm being unfair... What I would also say though, is that I do very much try to hold myself to these rules, as I did when I was working on gEDA, and PCB far more actively. Some general themes which I'll not drag up for individual patches... "//" comments are not PCB style. Nor is "if () {" in the core... it is: if (condition) { code(); more_code(); } NB: What you do matches more with the GTK HID, which (for some unknown reason) uses a slightly different coding style. Go figure. I don't like PCB's style, but I'd ask that you respect it like every other developer has done. We might decide to change it at some point, but I don't think one person should force this on us with a code dump. Considering a different coding style in a brand-new file is probably a reasonable compromise though... if it is a huge imposition to fix areas where you've written significant amounts of new code. Ideally, I'd say avoid mixing white-space changes with functional ones - it makes review harder. It is fine to change these things as you encounter them, just do it as a separate commit. (Having stgit helps no end here). Please don't automate a huge white-space change.. it would cause me NO END of merge pain - lets just try to improve things gradually as we work around certain areas of code. FWIW, My general rule of thumb, is I'll let myself make white-space changes within a few lines of a real code-change, if it is in the same function, and makes things look less ugly overall. (Obviously, fixing trailing white-space on lines with real code-changes is fine too). Anything more invasive, ought to be a separate patch (although I'm sure you'd be able to find commits where I've broken this "rule"). When we get to committing / merging, I'd prefer to see your simpler changes fixed (if required), and committed (cherry-picked?) on top of master - but its not super important. Do you have any thoughts on deleting branches once merged? (I dumped a list of branches, and spent a long time reviewing home/bkerin/drc_fixes before I realised it was already merged!) I had many comments on that one (some of which I now see you addressed afterwards)... TBH - we should really kill off pointer warping in a modern UI... it just doesn't fit any more - although this is probably not something which needs discussing now though. On to the specifics: "backtrace" Big NAK on merging unless you can show it builds and runs on Win32. (Or can be committed, but disabled by default). This is not the kind of code I like carrying - developers debug under gdb and valgrind (so it is of less use to them), and since we're good developers - we don't ship code that crashes - right? DJ suggested we could commit with the code disabled, which I'd be ok with. "home/bkerin/debugging_markers" I'm not keen on this, but don't have any strong objections. The main ones I had: Avoid use of assert(), especially to check bounds when "n" markers are being added - this is ugly - you just KNOW it will fire, and cause someone a headache. Why maximum 1042 markers? (Why not 1043, 666, ???) I may be a tiny bit biased, as I suspect this one will cause me merge pain when I come to rebase my own monster pile of patches on top of it. (I've done a fair bit of re-factoring of the HID / drawing interface). It also strikes me as the kind of thing I'd end up wanting to pull out (perhaps years down the line)... once we've finished using it. I personally tend to carry debug patches like this in stgit myself - not that I'm saying you need to do the same, but we'd be drowning in dead-code if I committed all the ones I ever used whilst developing new features. "home/bkerin/drc_warning_for_noobs" This one offends my sense of OCD, to leave a DRC warning on every design, which cannot be fixed! (But technically, it is ready for being cherry-picked). IMO, a better fix - if we care, is to actually run the rats-nest check underlying the "O" command (refactor if needed), to see whether any shorts or missing nets remain. Add DRC warning messages for "There are "n" shorted nets in the design, see .... for details", and "There are "n" missing nets n the design, see ... for details" - as required. "home/bkerin/require_c99" If you can present justification for why we want a C99 compiler, then fine - but small branches like this probably ought just to get cherry-picked into master, not merged. NAK if the reason is to enable laxer coding style, without a general decision that we want to adopt such a style. (That includes "//" comments, and I particularly dislike defining variables mid-function scope - if it helps, then your functions are too long!). NB: By "general discussion", I mean - we pick a project leader, and they decide... we can all imagine the length of mailing-list thread such a topic could generate for approximately zero positive benefit. I have not yet looked in depth at any of the code changes the big complex branches, "geometry_module", "geometry_module2" and "fix_drc_violation_locations". (Is one of the "geometry_module.." branches now redundant??) If these do what I think they do - then I'm looking forward to seeing the results. (Improving the DRC output is a really important task - one which has obviously been needed since back when we added the abstracted DRC with the better ability to preview the violation location). Skimming the commits in these branches, I think they might benefit from some clean-up before merging. Bear in mind - I've not done any code-review of the actual changes yet, I'm mostly looking at the branch structure, and churn. It appears that many of the patches sit upon commits duplicated in other branches (like the debug changes) - some of which migth need changes (e.g. no back-trace enabled by default unless it is portable), or might not want to end up in the repository. Looking at the commit comments, it appears that there are some points in the branch where the source won't even build (which has always been a big NO NO, for pushing code into master). I would expect that after each commit, the code should build, AND be fully functional. (This really helps with bisecting to find errors etc..). If I were you, I would pull that entire branch into stgit ("stg uncommit -n "), and start moving code about, re-factoring patches, until the deltas for each commit are clean, all commits build (and ideally work!). This strategy can be time-consuming (look how long many of my patches have sat), but I really believe it yields better code changes. This is a key difference between working with SCM systems like git (which let you prepare and finesse code changes), as compared to the bad-old-days of CVS, where developers would hack away on changes in a time-linear fashion - since managing branches and patch series was hard with those tools. I'm still keen to do an actual code-review of the changes in these bigger branches, but also wanted to get your thoughts on willingness to try out stgit, and/or shuffle patches around. I can help if required, and could - for example, put together an alternative stack-up of patches by way of example. Peter On 4 January 2016 at 00:15, Britton Kerin (britton DOT kerin AT gmail DOT com) [via geda-user AT delorie DOT com] wrote: > looks like email as I'm not able to get to computer at the moment. list > or not either is fine with me > On Jan 3, 2016 1:54 PM, "Peter Clifton (petercjclifton AT googlemail DOT com) > [via geda-user AT delorie DOT com]" wrote: > >> Hi Britton, >> >> I'm reading patches now, and am about on IRC. >> >> Currently just going through branch by branch, making some summaries - >> which I can share with you here, on IRC, or off-list. >> >> Peter >> >> >> On 3 January 2016 at 00:33, Britton Kerin (britton DOT kerin AT gmail DOT com) [via >> geda-user AT delorie DOT com] wrote: >> >>> >>> On Sat, Jan 2, 2016 at 12:53 PM, Peter Clifton ( >>> petercjclifton AT googlemail DOT com) [via geda-user AT delorie DOT com] < >>> geda-user AT delorie DOT com> wrote: >>> >>>> Excellent, >>>> >>>> I think doing some interactive review / chattting on IRC will probably >>>> be a fun way to look at this. I might ask you to take a look at some of >>>> mine too. (If we catch Bert or DJ too by any chance, that would also be >>>> cool). >>>> >>>> What timezone are you in? I'm GMT, but might not be up super early >>>> tomorrow after two busy days I've spent in London. >>>> >>> Alaska UTC-9. I'll set email to beep tomorrow >>> >> >> --001a113aca4e7891d0052878a7e7 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
I'm probably starting to run out o= f energy for tonight, having got chatting on IRC...

Before I ge= t started, I'm probably going to pick out some areas, and quote project= "rules" which I'm sure I've broken myself at one=20 time or other, either inadvertently, deliberately, or out of laziness! - Be= ar that in mind if you think I'm being unfair...

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>
Nor is "if () {" in the core... it is:

if (condition)
=C2=A0 {
=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 HID, which (for some unknown reason) uses a sligh= tly different coding style. Go figure. I don't like PCB's style, bu= t I'd ask that you respect it like every other developer has done. We m= ight decide to change it at some point, but I don't think one person sh= ould force this on us with a code dump. Considering a different coding styl= e in a brand-new file is probably a reasonable compromise though... if it i= s a huge imposition to fix areas where you've written significant amoun= ts of new code.

Ideally, I'd say avoid mixing white-s= pace changes with functional ones - it makes review harder. It is fine to c= hange these things as you encounter them, just do it as a separate commit. = (Having stgit helps no end here). Please don't automate a huge white-sp= ace change.. it would cause me NO END of merge pain - lets just try to impr= ove things gradually as we work around certain areas of code.

FWIW, = My general rule of thumb, is I'll let myself make white-space changes w= ithin a few lines of a real code-change, if it is in the same function, and= makes things look less ugly overall. (Obviously, fixing trailing white-spa= ce on lines with real code-changes is fine too). Anything more invasive, ou= ght to be a separate patch (although I'm sure you'd be able to find= commits where I've broken this "rule").


When we get to committing / merging, I'd prefer to see your simpler c= hanges fixed (if required), and committed (cherry-picked?) on top of master= - but its not super important.

Do you have any thoughts = on deleting branches once merged? (I dumped a list of branches, and spent a= long time reviewing home/bkerin/drc_fixes before I realised it was already= merged!) I had many comments on that one (some of which I now see you addr= essed afterwards)... TBH - we should really kill off pointer warping in a m= odern UI... it just doesn't fit any more - although this is probably no= t something which needs discussing now though.


=
On to the specifics:


"= ;backtrace"

Big NAK on merging unless you can show it builds an= d runs on Win32. (Or can be committed, but disabled by default).

Thi= s is not the kind of code I like carrying - developers debug under gdb and = valgrind (so it is of less use to them), and since we're good developer= s - we don't ship code that crashes - right?

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


&quo= t;home/bkerin/debugging_markers"

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

The main ones I had:
= =C2=A0 Avoid use of assert(), especially to check bounds when "n"= markers are being added - this is ugly - you just KNOW it will fire, and c= ause someone a headache.
=C2=A0 Why maximum 1042 markers? (Why not = 1043, 666, ???)

I may be a tiny bit biased, as I suspect = this one will cause me merge pain when I come to rebase my own monster pile= of patches on top of it. (I've done a fair bit of re-factoring of the = HID / drawing interface).

It also strikes me as the kind = of thing I'd end up wanting to pull out (perhaps years down the line)..= . once we've finished using it. I personally tend to carry debug patche= s like this in stgit myself - not that I'm saying you need to do the sa= me, but we'd be drowning in dead-code if I committed all the ones I eve= r used whilst developing new features.



"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 techn= ically, it is ready for being cherry-picked).

IMO, a bett= er fix - if we care, is to actually run the rats-nest check underlying the = "O" command (refactor if needed), to see whether any shorts or mi= ssing nets remain. Add DRC warning messages for "There are "n&quo= t; shorted nets in the design, see .... for details", and "There = are "n" missing nets n the design, see ... for details" - as= required.



"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-picke= d into master, not merged.

NAK if the reason is to enable laxer codi= ng style, without a general decision that we want to adopt such a style. (T= hat includes "//" comments, and I particularly dislike defining v= ariables mid-function scope - if it helps, then your functions are too long= !).

NB: By "general discussion", I mean - we pi= ck a project leader, and=20 they decide... we can all imagine the length of mailing-list thread such a topic could generate for approximately zero positive benefit.
<= div>



I have not yet looked in depth at any of the= code changes the big complex branches, "geometry_module", "= geometry_module2" and "fix_drc_violation_locations". (Is one= of the "geometry_module.." branches now redundant??)

If t= hese do what I think they do - then I'm looking forward to seeing the r= esults. (Improving the DRC output is a really important task - one which ha= s obviously been needed since back when we added the abstracted DRC with th= e better ability to preview the violation location).

Skimming the co= mmits in these branches, I think they might benefit from some clean-up befo= re merging. Bear in mind - I've not done any code-review of the actual = changes yet, I'm mostly looking at the branch structure, and churn.
=
It appears that many of the patches sit upon commits duplica= ted in other branches (like the debug changes) - some of which migth need c= hanges (e.g. no back-trace enabled by default unless it is portable), or mi= ght not want to end up in the repository.

Looking at the = commit comments, it appears that there are some points in the branch where = the source won't even build (which has always been a big NO NO, for pus= hing code into master). I would expect that after each commit, the code sho= uld build, AND be fully functional. (This really helps with bisecting to fi= nd errors etc..).

If I were you, I would pull that entire= branch into stgit ("stg uncommit -n <number of patches>"),= and start moving code about, re-factoring patches, until the deltas for ea= ch commit are clean, all commits build (and ideally work!).

This strategy can be time-consuming (look how long many of my patches ha= ve sat), but I really believe it yields better code changes. This is a key = difference between working with SCM systems like git (which let you prepare= and finesse code changes), as compared to the bad-old-days of CVS, where d= evelopers would hack away on changes in a time-linear fashion - since manag= ing branches and patch series was hard with those tools.


<= div>I'm still keen to do an actual code-review of the changes in these = bigger branches, but also wanted to get your thoughts on willingness to try= out stgit, and/or shuffle patches around.

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

Peter

=

On 4 Januar= y 2016 at 00:15, Britton Kerin (= britton DOT kerin AT gmail DOT com) [via = geda-user AT delorie DOT com] <geda-user AT delorie DOT com> wrote= :

looks like email as I= 9;m not able to get to computer at the moment.=C2=A0 list or not either is = fine with me

On Jan 3, 2016 1:54 PM, "Peter Clifton (petercjclif= ton AT googlemail DOT com) [via geda-user AT delorie DOT com]" <geda-user AT delorie DOT com> wrote:
Hi Britton,

I'm reading patches now, and am about = on IRC.

Currently just going through branch by branch, making = some summaries - which I can share with you here, on IRC, or off-list.
<= br>
Peter


On 3 January 2016 at 00:33, Britton Kerin = (britton.kerin= @gmail.com) [via geda-user AT delorie DOT com] <geda-user AT delorie DOT com> = wrote:

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

Excellent,

I think doing some interactive review / chattting on IRC wil= l probably be a fun way to look at this. I might ask you to take a look at = some of mine too. (If we catch Bert or DJ too by any chance, that would als= o be cool).

What timezone are you in? I'm GMT, but might not be up s= uper early tomorrow after two busy days I've spent in London.

Alaska UTC-9.=C2=A0 I'll set email to beep tomorrow<= br>


--001a113aca4e7891d0052878a7e7--