Reviewers: hahnjo,
Message:
I don't think we should go this hacky way. For Clang
I'm already fixing this
issue with https://sourceforge.net/p/testlilyissues/issues/5608/
(Especially the confdefs.h should be fixed correctly,
by untangling AutoConf and StepMake and not define
the macro twice
Please extend this patch to cover gcc, too.
https://codereview.appspot.com/573270043/
LGTM, thanks!
https://codereview.appspot.com/579140043/diff/549240043/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):
https://codereview.appspot.com/579140043/diff/549240043/ly/music-functions-init.ly#newcode243
ly/music-functions-init.ly:243: (if (and (number? cbn) (not
:-)
https://codereview.appspot.com/581270043/
LGTM
https://codereview.appspot.com/573270043/diff/549210045/configure.ac
File configure.ac (right):
https://codereview.appspot.com/573270043/diff/549210045/configure.ac#newcode212
configure.ac:212: # options. Unfortunately GCC doesn't support this
options, so we just
s/options/option/
And
Wait, this tiny patch makes lilypond compilable with clang, passing the
whole test suite?
https://codereview.appspot.com/559250043/
And yes, this works for me, at least on Linux (didn't test FreeBSD yet
and don't
own a Mac). Plus it passes a full check with a test-baseline generated
by GCC,
so I'm pretty confident it's also working correctly.
Ho humm :-) David K?
https://codereview.appspot.com/559250043/
Great! Jonas, please add comments to the source code that points to the
relevant gcc vs. clang discussion.
https://codereview.appspot.com/559250043/
> There should always a comma after 'i.e.', as far as I know.
Nope. It's grammatical dogma (in the same vein as not
starting sentences with the words 'And' or 'Because).
I think commas after these abbreviations look ugly.
I'm not a native speaker, but I think exactly the opposite, given
LGTM
https://codereview.appspot.com/571280043/
Mhmm, I'm not happy with that. What I can imagine is to *prefer* Python
3.x over 2.x.
Is there a reason to enforce 3.x? I thought that all of your recent
work was to ensure that the scripts work both with 2.x and 3.x – am I
missing something?
https://codereview.appspot.com/545370043/
LGTM
https://codereview.appspot.com/559340043/
> > I'm not a native speaker, but I think exactly the opposite,
> > given that 'i.e.' means 'that is' and 'e.g.' means 'for
> > example'.
I don't understand your point. [...]
It's a matter of style, not a matter of grammar correctness.
I've never understood the point of two spaces after a
I don't mind if we follow a different style. What would
you recommend?
Of course, changing the style would mean that all occurrences should be
fixed.
https://codereview.appspot.com/551270043/
LGTM, thanks!
https://codereview.appspot.com/551270043/diff/583260043/Documentation/notation/editorial.itely
File Documentation/notation/editorial.itely (right):
https://codereview.appspot.com/551270043/diff/583260043/Documentation/notation/editorial.itely#newcode576
As I said on the mailing list multiple times, it's actually
a ton of work to make the scripts work with Python 2 _and_
Python 3 at the same time. [...]
I've forgotten that you've already added Python 3.x support to gub, so I
withdraw my comments :-)
https://codereview.appspot.com/545370043/
LGTM
https://codereview.appspot.com/551260043/
LGTM, with some minor remarks.
https://codereview.appspot.com/551270043/diff/575420043/Documentation/notation/editorial.itely
File Documentation/notation/editorial.itely (right):
https://codereview.appspot.com/551270043/diff/575420043/Documentation/notation/editorial.itely#newcode496
LGTM
https://codereview.appspot.com/549560043/
LGTM
https://codereview.appspot.com/565660044/
LGTM, thanks!
https://codereview.appspot.com/559470043/
LGTM, thanks!
https://codereview.appspot.com/581630043/
LGTM, thanks!
https://codereview.appspot.com/581700043/diff/569410044/lily/ttf.cc
File lily/ttf.cc (right):
https://codereview.appspot.com/581700043/diff/569410044/lily/ttf.cc#newcode95
lily/ttf.cc:95:
I think the second empty line can be removed.
LGTM
https://codereview.appspot.com/551510043/
> It sounds like actual manipulation of that property
> by the user would interfere with the implementation
> of french-beaming. So maybe just mark/sort it as an
> internal property and only regtest french-beaming as
> such?
If this is the intention, yes.
Reviewers: hahnjo,
Message:
> Do we really want that many "no"s in the output
Currently, you get this (in one long line):
checking for guile-1.8 >= 1.8.2... checking for guile-2.2 >= 2.2.0...
checking for guile-2.0 >= 2.0.7... Package guile-2.0 was not found in
the pkg-config search path.
LGTM. Please feel free to ignore (most of) my remarks if you consider
such nitpicking as unnecessary :-)
https://codereview.appspot.com/563630043/diff/571770043/lily/include/page-breaking.hh
File lily/include/page-breaking.hh (right):
> How about
> * beamed-stem-french-adjustment
> * french-beaming-stem-adjustment
> …?
I like the second one.
https://codereview.appspot.com/557500043/
Very nice, thanks! LGTM.
https://codereview.appspot.com/571780043/diff/549620043/mf/feta-arrowheads.mf
File mf/feta-arrowheads.mf (right):
https://codereview.appspot.com/571780043/diff/549620043/mf/feta-arrowheads.mf#newcode58
mf/feta-arrowheads.mf:58: y5 = 0;
Please use an indentation of 8
LGTM; some minor nits
https://codereview.appspot.com/553580043/diff/571780045/mf/GNUmakefile
File mf/GNUmakefile (right):
https://codereview.appspot.com/553580043/diff/571780045/mf/GNUmakefile#newcode130
mf/GNUmakefile:130: $(outdir)/emmentaler-brace.otf:
emmentaler-brace.fontforge.py
LGTM
https://codereview.appspot.com/547690053/
> > Can we assume that FontForge's python support and is
> > always enabled? Shall we check this?
>
> the FF page doesn't say that python is optional.
It's a build option in both the old (configure) and new (cmake)
builds...
https://codereview.appspot.com/553580043/
LGTM, thanks!
https://codereview.appspot.com/571780043/
LGTM
https://codereview.appspot.com/549680043/
LGTM
https://codereview.appspot.com/563650043/
LGTM.
https://codereview.appspot.com/555370043/
LGTM
https://codereview.appspot.com/579340043/
LGTM
https://codereview.appspot.com/565720043/
LGTM
https://codereview.appspot.com/569490044/
Very nice, thanks! From visual inspection, LGTM, with only minor nits.
https://codereview.appspot.com/567340043/diff/565730043/scripts/build/fix-docsize.sh
File scripts/build/fix-docsize.sh (right):
https://codereview.appspot.com/567340043/diff/565730043/scripts/build/fix-docsize.sh#newcode37
Very nice, thanks! LGTM.
https://codereview.appspot.com/573570043/
LGTM. Very nice, thanks! Some minor nits only.
https://codereview.appspot.com/557500043/diff/551490044/Documentation/changes.tely
File Documentation/changes.tely (right):
https://codereview.appspot.com/557500043/diff/551490044/Documentation/changes.tely#newcode67
> It's not urgent, is it?
No, not at all. It just appeared to be a trivial change.
https://codereview.appspot.com/545640044/
> > s/i.e./i.e.,/
>
> Ah, then I suppose the changes.tely language is American English...
Yes, for the whole documentation we use (more or less) American spelling
rules. If you really wanted 'i.e.' without a trailing comma, it would
be necessary to write 'i.e.@:'. Otherwise makeinfo inserts
LGTM. Please directly push.
https://codereview.appspot.com/545640044/
not happy with indentation yet...
https://codereview.appspot.com/571780043/diff/549620043/mf/feta-scripts.mf
File mf/feta-scripts.mf (left):
https://codereview.appspot.com/571780043/diff/549620043/mf/feta-scripts.mf#oldcode999
mf/feta-scripts.mf:999: .. z2l
> Nevertheless, I've changed the
I'm quite sure that you have more experience with C++ than me, however,
I'm not really happy with this change since it makes the code
substantially uglier. Isn't there another solution for the problem?
https://codereview.appspot.com/579240043/diff/577360043/flower/include/file-name.hh
File
For which clang-format version is this format file? It doesn't work
with 7.0.1, for example. Please add this information to the file.
https://codereview.appspot.com/561340043/
LGTM, thanks!
https://codereview.appspot.com/563420043/diff/547500043/lily/grob.cc
File lily/grob.cc (right):
https://codereview.appspot.com/563420043/diff/547500043/lily/grob.cc#newcode492
lily/grob.cc:492: return real_ext;
whitespace
> > Do you want me to make this change?
> > I don't have a strong opinion on this one,
> > I just left the name from the original message.
>
> In case a second opinion is useful, I'll give mine.
> I see Werner's point, but I also think
> the current name is clear enough,
> so I wouldn't blame you
LGTM
https://codereview.appspot.com/563400065/
> Why is this ugly? std::string is really the name of the class.
No question, but I would prefer
struct xxx {
using std:string;
string foo;
string bar;
string baz;
}
}
to
struct xxx {
std::string foo;
std::string bar;
std::string baz;
}
for example, to
LGTM
https://codereview.appspot.com/571430048/
LGTM
https://codereview.appspot.com/561350044/
LGTM
https://codereview.appspot.com/553430047/diff/573430051/python/lilylib.py
File python/lilylib.py (right):
https://codereview.appspot.com/553430047/diff/573430051/python/lilylib.py#newcode257
python/lilylib.py:257: sys.stderr.write('command failed: %s\n' % cmd)
With space before the '(' or
LGTM
https://codereview.appspot.com/565560043/
> I don't like this methodology,
+1
> I prefer what Dan does right now (btw thanks for
> working on this!): Pick a warning, investigate
> the issue and fix it correctly.
I currently add `CXXFLAGS="-Wno-sign-conversion"` while calling make;
this reduces the amount of warnings with clang to a
Next round of nits :-)
https://codereview.appspot.com/561360043/diff/575590043/Documentation/contributor/quick-start.itexi
File Documentation/contributor/quick-start.itexi (left):
https://codereview.appspot.com/561360043/diff/575590043/Documentation/contributor/quick-start.itexi#oldcode103
> > Documentation/contributor/quick-start.itexi:46:
> > create it from the sources located in the
> > @code{/mkosi} subdirectory
> > s/@code/@file/.
>
> Slightly puzzled about this one. You suggested
> '@code' in your first review, didn't you?
Yep. A mistake, sorry. '@file' is the right one
And hopefully some final nits. Thanks for your patience!
https://codereview.appspot.com/561360043/diff/565550050/Documentation/contributor/quick-start.itexi
File Documentation/contributor/quick-start.itexi (right):
An important nit to check...
Besides this, LGTM, thanks!
https://codereview.appspot.com/579280043/diff/567170044/Documentation/notation/input.itely
File Documentation/notation/input.itely (right):
LGTM
https://codereview.appspot.com/557270049/
> you need to read the comment at the top of the file too..
Well, ...
> I think this function should not have been pulbic,
> and suspect it is only so some of the macros can work.
... even a non-public function should have an understandable doc string,
so thanks for the fix!
uot;Lookup procedure in the current module, or
return
> #f"
> On 2020/02/01 07:04:11, lemzwerg wrote:
> > s/Lookup/Look up/
>
> Done.
>
>
https://codereview.appspot.com/581570043/diff/563450043/scm/markup-macros.scm#newcode427
> scm/markup-macros.scm:427: "
Very nice, thanks! LGTM
https://codereview.appspot.com/575600044/
> In `-std=c++11`, most POSIX functions/definitions cannot
> be used.
> Also `putenv ()` and `chroot ()` cannot be used.
What about using
AC_USE_SYSTEM_EXTENSIONS
instead (in `configure.in`) to activate many POSIX functionality?
https://codereview.appspot.com/579270051/
LGTM
https://codereview.appspot.com/553490051/
LGTM now, thanks!
https://codereview.appspot.com/579270051/
LGTM
https://codereview.appspot.com/551430044/
On 2020/02/02 15:17:04, hanwenn wrote:
> *say
Yep, LGTM.
https://codereview.appspot.com/581570043/
LGTM
https://codereview.appspot.com/559450053/diff/547570044/lily/accidental-placement.cc
File lily/accidental-placement.cc (right):
https://codereview.appspot.com/559450053/diff/547570044/lily/accidental-placement.cc#newcode62
lily/accidental-placement.cc:62: scm_from_long (stagger ?
Grammar nit.
https://codereview.appspot.com/577390043/diff/573430045/lily/open-type-font.cc
File lily/open-type-font.cc (right):
https://codereview.appspot.com/577390043/diff/573430045/lily/open-type-font.cc#newcode238
lily/open-type-font.cc:238: debug_output (_f ("Replace font name from %s
to
LGTM
https://codereview.appspot.com/573420043/diff/575530043/lily/main.cc
File lily/main.cc (left):
https://codereview.appspot.com/573420043/diff/575530043/lily/main.cc#oldcode515
lily/main.cc:515: // SCM result = scm_call_1 (
I guess you are submitting two commits as one Rietveld issue, right?
LGTM
https://codereview.appspot.com/547470043/
Would be nice to have a regtest so that there is an example how to use
it.
https://codereview.appspot.com/561310045/
LGTM, thanks
https://codereview.appspot.com/547470044/
LGTM, thanks
https://codereview.appspot.com/571420055/diff/571400046/configure.ac
File configure.ac (right):
https://codereview.appspot.com/571420055/diff/571400046/configure.ac#newcode391
configure.ac:391: ["(Using Ghostscript >= 9.20 together with Extract
PDFmark"])
I suggest to use the
OK, point taken. Thanks for all the responses.
Well, then the ugly way :-)
https://codereview.appspot.com/579240043/
LGTM
https://codereview.appspot.com/573440043/
Some more nits. Thanks for working on this!
https://codereview.appspot.com/561360043/diff/547520043/Documentation/contributor/quick-start.itexi
File Documentation/contributor/quick-start.itexi (right):
> just the overrides
The new version works just fine. Shall I still inspect the previous
version for problematic statements?
The output looks good. BTW, for the sake of Emacs, it would be nice if
two spaces after a full dot could be retained in comments. Does such an
option exist?
A nit.
https://codereview.appspot.com/579280043/diff/579290043/Documentation/notation/input.itely
File Documentation/notation/input.itely (right):
https://codereview.appspot.com/579280043/diff/579290043/Documentation/notation/input.itely#newcode464
Documentation/notation/input.itely:464:
LGTM
https://codereview.appspot.com/547540043/diff/561380043/lily/main.cc
File lily/main.cc (right):
https://codereview.appspot.com/547540043/diff/561380043/lily/main.cc#newcode772
lily/main.cc:772: sane_putenv("GC_INITIAL_HEAP_SIZE", "40M", false);
Do you think it makes sense to mention this
> If Werner likes it, I'm fine with it.
I do like it, and it is completely non-intrusive since it gets used
locally only for those people who set up a proper git hook (or call
`clang-format` manually).
https://codereview.appspot.com/561340043/
Mhmm, wouldn't it be better to simply define `M_PI` if it is not
defined? GNU options might not be available with other compilers...
https://codereview.appspot.com/579270051/
> Werner, may I ask you to have a look?
What exactly shall I check?
https://codereview.appspot.com/571640044/
> à vo -- cê uma can -- ção legal
>
> I don't know how to change it, so that
> http://lsr.di.unimi.it/LSR/Snippet?id=600
> shows the same as in Han-Wen's patch here.
This seems to be a bug on LSR webpage: What you see is double-encoded
UTF-8 (see
LGTM, thanks!
https://codereview.appspot.com/577520043/diff/555310043/python/musicexp.py
File python/musicexp.py (left):
https://codereview.appspot.com/577520043/diff/555310043/python/musicexp.py#oldcode365
python/musicexp.py:365: "nederlands": pitch_nederlands,
You are sorting the entries
Very nice, thanks! LGTM
https://codereview.appspot.com/583550043/
LGTM
https://codereview.appspot.com/571690043/
> > could we check the version number instead?
>
> No, the header doesn't have version information.
So we need both a recent flex program and a recent flex header, right?
For the former a version test would be easy, see
> > I'd love to see more opinions about comments #12 and #14.
> > Otherwise I'd object to this part of the patch
>
> I agree with the objection raised in #12
+1
https://codereview.appspot.com/579280043/
LGTM
https://codereview.appspot.com/547610043/diff/567190043/scm/define-note-names.scm
File scm/define-note-names.scm (right):
https://codereview.appspot.com/547610043/diff/567190043/scm/define-note-names.scm#newcode302
scm/define-note-names.scm:302: (heh . ,(ly:make-pitch -1 6 SEMI-FLAT))
What
> > What about providing an alias for the old name 'beh'
> for backward compatibility?
>
> I'd rather not since it so glaringly wrong. [...]
OK, but then we need either a NEWS entry or a convert-ly rule (or both)
IMHO.
https://codereview.appspot.com/547610043/
LGTM
https://codereview.appspot.com/583510045/
LGTM, thanks.
https://codereview.appspot.com/583510043/
Good idea, thanks! LGTM.
Apropos .lo files: We can remove STEPMAKE_LIBTOOL in `aclocal.m4` since
it isn't used. I think this change would probably fit into this issue,
too.
https://codereview.appspot.com/555300044/
LGTM
https://codereview.appspot.com/561460044/
LGTM. However, the use of the pkgconfig environment variable(s) should
be documented.
https://codereview.appspot.com/569390043/
LGTM
https://codereview.appspot.com/555330043/
501 - 600 of 713 matches
Mail list logo
