Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(304)

Issue 5375051: Keep yaffut from attempting to demangle. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 7 months ago by dak
Modified:
7 years, 7 months ago
Reviewers:
pkx166h, Graham Percival, janneke, janneke, reinhold
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Keep yaffut from attempting to demangle. Issue 1875 One can get the pretty output by piping the results through "c++filt -t", but alas, the stepmake stuff is so totally inscrutable that I have no idea how one would make the test target do that. But non-demangled output is better than a complete failure.

Patch Set 1 #

Patch Set 2 : I wasted about 4 hours of work to keep demangling in _if_ it works. yaffut.hh should be thrown out. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+29 lines, -2 lines) Patch
M config.hh.in View 1 1 chunk +4 lines, -0 lines 0 comments Download
M configure.in View 1 1 chunk +12 lines, -0 lines 0 comments Download
M flower/include/yaffut.hh View 1 3 chunks +8 lines, -0 lines 0 comments Download
M flower/test-file-name.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M flower/test-file-path.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M flower/test-std.cc View 1 1 chunk +2 lines, -0 lines 0 comments Download
M flower/test-string.cc View 1 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 7
pkx166h
Passes make and no reg test diffs james
7 years, 7 months ago (2011-11-10 20:07:34 UTC) #1
janneke
On 2011/11/10 20:07:34, J_lowe wrote: > Passes make and no reg test diffs +// Modified ...
7 years, 7 months ago (2011-11-11 06:48:28 UTC) #2
dak
On 2011/11/11 06:48:28, jan.nieuwenhuizen wrote: > On 2011/11/10 20:07:34, J_lowe wrote: > > Passes make ...
7 years, 7 months ago (2011-11-11 08:31:05 UTC) #3
reinhold_fam.tuwien.ac.at
On 2011-11-11 09:31, dak@gnu.org wrote: >> Remember that yaffut is a cross platform test suite, ...
7 years, 7 months ago (2011-11-11 11:21:07 UTC) #4
dak
Reinhold Kainhofer <reinhold@fam.tuwien.ac.at> writes: > On 2011-11-11 09:31, dak@gnu.org wrote: >>> Remember that yaffut is ...
7 years, 7 months ago (2011-11-11 11:32:21 UTC) #5
janneke_gnu.org
David Kastrup writes: > Issue 1875. Ah, that's the bit I missed. Just an artifact ...
7 years, 7 months ago (2011-11-11 13:42:58 UTC) #6
Graham Percival
7 years, 7 months ago (2011-11-11 16:46:24 UTC) #7
On Fri, Nov 11, 2011 at 02:42:55PM +0100, Jan Nieuwenhuizen wrote:
> David Kastrup writes:
> 
> > The patch in its current state does not affect a working test suite, and
> > it lets "make check" finish with slightly less readable output when
> > cxxabi.h breaks g++ 4.6.  So it is _strictly_ an improvement.
> 
> Then commit it already!

Classy.

(the below is NOT directed at David, who is an absolutely
fantastic developer)


"one programmer says it's good.  So let's just bypass the review
process and commit it."

Might I remind you that almost everybody wants to have stable
release more often?  And that the biggest cause of not having
stable releases are regressions?  And that the best time to fix
regressions are *before* they happen?  And that code reviews have
been found to be the best single factor for catching bugs?  And
that nobody (apart from maybe you) actually knows what yaffut is
doing or how it works?

David is good.  Really good.  But he's also human.  Maybe he added
an extra semicolon somewhere that makes the code look ok, but blow
up in certain circumstances.  (I've spent at least 20 hours of my
life on semicolon bugs)
Maybe his changes work great on his machine, but they'll fail when
using a different version of gcc.  Maybe he just has some variable
names that are unclear, or maybe a one-line comment would clarify
something... I mean, after staring at his change to yaffut.hh for
a while, I gather that it's constructing a name for a type... but
if I'd seen
  // construct a name for a type of variable
at the top of the demangle() function, I wouldn't have to spend a
minute or two puzzling through stuff like
  std::string name (ptr ? ptr : "", ptr ? strlen (ptr) : 0);

Of course the lack of a comment like that isn't David's fault;
it's the fault of whoever wrote that code in the first place.  But
this would be a good opportunity to clarify that, for future
programmers.


Not all programmers are as good as you and Han-Wen.  But sadly,
lilypond is being maintained by us stupid programmers.
(although we *do* still have some genius programmers as well, so
don't think that I'm insulting all lilypond developers who read
this.  I'm only claiming that *I'm* a stupid programmer, not you)

If lilypond was alpha quality, it wouldn't matter if things
magically broke.  It's alpha, who cares!

If lilypond was a commercial software product, it wouldn't matter
(much) if we had difficult-to-read code.  If the code can only be
read by developers with 5 years of C++ experience, so what?  ok,
those developers might cost twice as much as junior programmers,
but that's just a business decision -- how much time should the
current programmers spend writing clear code, vs. how much do we
want to pay for hiring new developers.

Unfortunately, most people expect (or at least want) lilypond to
be stable, and we have no budget for hiring experienced
developers.


The 48-hour "patch countdown" is designed to give people a chance
to address both problems -- does the code work, and is it
*obvious* how the code works.  Granted, we rarely take advantage
of the review period to actually discuss what the code is doing
and request more comments... but this *does* happen occasionally,
and I definitely think it's worth keeping the *opportunity* for
idiots like me to ask for help in understanding the code.

- Graham
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b