LGTM. This issue also adds some scheme code and a conversion rule –
they are different commits, right?
https://codereview.appspot.com/560030044/diff/557800043/scm/define-context-properties.scm
File scm/define-context-properties.scm (right):
https://codereview.appspot.com/560030044/diff/55780004
> > I don't fully understand this code.
>
> that is an ominous comment to come from the Freetype
> author. I had assumed you reviewed the existing code
> as well.
I have two difficulties: I'm not good at C++ and Guile stuff, and it's
not completely clear to me what the code wants to achieve. I
*Much* better to read and understand, thanks! LGTM now.
https://codereview.appspot.com/566080043/
LGTM
https://codereview.appspot.com/573150043/
Thanks for the corrections.
https://codereview.appspot.com/566930043/diff/551130043/Documentation/contributor/source-code.itexi
File Documentation/contributor/source-code.itexi (right):
https://codereview.appspot.com/566930043/diff/551130043/Documentation/contributor/source-code.itexi#newcode22
LGTM. Your changes are two commits, right?
https://codereview.appspot.com/575180043/diff/559180043/aclocal.m4
File aclocal.m4 (right):
https://codereview.appspot.com/575180043/diff/559180043/aclocal.m4#newcode274
aclocal.m4:274: # "print a verbose error report and exit the program"
Why quotati
LGTM
https://codereview.appspot.com/545100043/diff/545110043/scripts/build/output-distance.py
File scripts/build/output-distance.py (right):
https://codereview.appspot.com/545100043/diff/545110043/scripts/build/output-distance.py#newcode100
scripts/build/output-distance.py:100: system ('convert
LGTM
https://codereview.appspot.com/545100043/
LGTM
https://codereview.appspot.com/573210043/
LGTM, thanks. In case there are problems, we might use `printf`
instead, which has less portability problems.
https://codereview.appspot.com/575260043/
LGTM
https://codereview.appspot.com/563260043/
LGTM, thanks.
In the patch description: s/divison/division/
https://codereview.appspot.com/554970043/
LGTM, and thanks for cleaning up those stuff!
https://codereview.appspot.com/559240043/
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/
> Great! Jonas, please add comments to the source code
>that points to the relevant gcc vs. clang discussion.
Where exactly would you put such comment? IMO we don't
need to justify writing correct code (or at least code
that both compilers accept eventually).
Fair point, so please forget my
LGTM from reading the code without testing. Thanks!
https://codereview.appspot.com/571180043/diff/565230043/input/regression/measure-spanner.ly
File input/regression/measure-spanner.ly (right):
https://codereview.appspot.com/571180043/diff/565230043/input/regression/measure-spanner.ly#newcode5
LGTM
https://codereview.appspot.com/577110043/
Reviewers: hahnjo,
Message:
I think this should be `#if HAVE_LIBGUILE18_H` because it's
always defined AFAICS. This also applies to all other uses.
Yep, found it at the same time as you wrote your review :-)
Thanks for checking!
Description:
Allow use of `libguile18.h` instead of `libguile.
:-)
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 ple
From visual inspection: LGTM, thanks!
https://codereview.appspot.com/575320043/
LGTM
https://codereview.appspot.com/577130043/
Very nice, LGTM!
https://codereview.appspot.com/575330043/
LGTM. I will hopefully run full builds soon again...
https://codereview.appspot.com/545270043/
LGTM
https://codereview.appspot.com/579120043/
LGTM, thanks!
https://codereview.appspot.com/573280043/
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 i
Please extend this patch to cover gcc, too.
https://codereview.appspot.com/573270043/
AFAICT this already works with GCC because it just
returns an error code: [...]
OK. This deserves a comment IMHO.
https://codereview.appspot.com/573270043/
LGTM. The goal is to have less dynamic casts, right?
https://codereview.appspot.com/545280043/
LGTM
https://codereview.appspot.com/549220043/
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 (=
LGTM
https://codereview.appspot.com/551240043/
LGTM, thanks!
https://codereview.appspot.com/571190043/diff/559290043/lily/system.cc
File lily/system.cc (right):
https://codereview.appspot.com/571190043/diff/559290043/lily/system.cc#newcode525
lily/system.cc:525: {
Is there a reason for grouping the code into additional braces?
https://code
LGTM
https://codereview.appspot.com/545320043/diff/581320043/flower/include/yaffut.hh
File flower/include/yaffut.hh (right):
https://codereview.appspot.com/545320043/diff/581320043/flower/include/yaffut.hh#newcode146
flower/include/yaffut.hh:146: // TODO: Once we are willing to require
C++11, u
Last time I had a short look, it seems LilyPond is setting
no -std= at all. This basically means we get the compiler
defaults, possibly changing with the next compiler release.
I think we should actually pass in something to avoid
problems right away (-std=c++11 or =gnu++11 ?).
Good idea. In c
LGTM
https://codereview.appspot.com/571210043/
LGTM
https://codereview.appspot.com/549270043/
LGTM
https://codereview.appspot.com/555070043/
LGTM
https://codereview.appspot.com/583180043/
Reviewers: Dan Eble,
Message:
I see many cases in the regression tests that this
change fails to improve, for example:
In input/regression/rehearsal-mark-formatters.ly,
the new position of the "H" mark is an improvement,
but the new position of the "CC" mark isn't.
Yes. This is unavoidable
Is there a major benefit from using C++14? Not saying
that I wouldn't be in favor, but if we can get C++11
for free...
C++14 is definitely too new. For example, TeXLive is going to replace
the poppler library with another library written in C because poppler
requires C++14, which too many syste
LGTM
https://codereview.appspot.com/565320043/
LGTM, thanks! Fixing those warnings is a great job.
https://codereview.appspot.com/583200043/
LGTM
https://codereview.appspot.com/561240043/
Somewhat weirdly, if we are making some standard
definite in the calling options, I'd somehow be
happier with C++11.
+1. AFAICS, C++11 is a major target for virtually all C++ compilers.
https://codereview.appspot.com/545320043/
LGTM, thanks!
https://codereview.appspot.com/553310045/
One early comment :-)
https://codereview.appspot.com/547340043/diff/575390044/lily/break-alignment-interface.cc
File lily/break-alignment-interface.cc (right):
https://codereview.appspot.com/547340043/diff/575390044/lily/break-alignment-interface.cc#newcode328
lily/break-alignment-interface.cc:
See https://codereview.appspot.com/547340043/ .
Looks promising, thanks!
https://codereview.appspot.com/553290043/
LGTM, thanks.
https://codereview.appspot.com/571250043/
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
Documentat
> 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 that
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/
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/559340043/
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
Documentation/notation/edit
> > 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/551300043/
Thanks for the review. I agree with all your points.
https://codereview.appspot.com/581400043/
LGTM, thanks!
https://codereview.appspot.com/561290043/diff/583280043/lily/context.cc
File lily/context.cc (right):
https://codereview.appspot.com/561290043/diff/583280043/lily/context.cc#newcode116
lily/context.cc:116: if (Context *score = gthis->get_score_context ())
Not sure whether compiler
LGTM, thanks!
https://codereview.appspot.com/577280043/
LGTM, thanks.
https://codereview.appspot.com/569220043/
LGTM, thanks!
https://codereview.appspot.com/583320043/diff/581420044/flower/file-name.cc
File flower/file-name.cc (right):
https://codereview.appspot.com/583320043/diff/581420044/flower/file-name.cc#newcode124
flower/file-name.cc:124: s += ext_;
any reason to use two lines? In other files you
s += EXTSEP; // append a char to s
s += ext_; // append a string to s
— vs. —
s += EXTSEP + ext_;
\___/
make a string
\/
append it to s
Either one could involve memory allocation, but I deemed it more
likely in the latter case. I don't have reason to
Only one minor nit :-) Thanks a lot for your contribution!
https://codereview.appspot.com/583340043/diff/571320043/Documentation/notation/text.itely
File Documentation/notation/text.itely (right):
https://codereview.appspot.com/583340043/diff/571320043/Documentation/notation/text.itely#newcode
LGTM, thanks! And I think that `ssize_t` is a good type in general
since it allows to use the value -1 as a special flag.
https://codereview.appspot.com/567090043/
LGTM, thanks!
https://codereview.appspot.com/577320043/
LGTM, of course. Please commit such simple, trivial, and
uncontroversial changes directly to the git repository.
https://codereview.appspot.com/581480043/
LGTM, thanks.
https://codereview.appspot.com/551380043/
LGTM
https://codereview.appspot.com/581510043/
From inspection only: LGTM, thanks!
https://codereview.appspot.com/573400043/diff/549420043/lily/includable-lexer.cc
File lily/includable-lexer.cc (right):
https://codereview.appspot.com/573400043/diff/549420043/lily/includable-lexer.cc#newcode74
lily/includable-lexer.cc:74: (current_dir.length
From inspection only: LGTM, thanks!
https://codereview.appspot.com/557190043/diff/581490044/lily/lookup.cc
File lily/lookup.cc (right):
https://codereview.appspot.com/557190043/diff/581490044/lily/lookup.cc#newcode314
lily/lookup.cc:314: int i0 = (int) i;
Maybe 'int(i)' instead?
https://codere
LGTM, thanks!
https://codereview.appspot.com/555160043/
Very nice, thanks!
https://codereview.appspot.com/571380043/
Some minor nits...
https://codereview.appspot.com/555160043/diff/549440043/lily/slur.cc
File lily/slur.cc (right):
https://codereview.appspot.com/555160043/diff/549440043/lily/slur.cc#newcode495
lily/slur.cc:495: "Slurs are formatted by a number of combination of
left/right "
s/combination/combi
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 flowe
> 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 incr
OK, point taken. Thanks for all the responses.
Well, then the ugly way :-)
https://codereview.appspot.com/579240043/
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?
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/547470043/
LGTM, thanks
https://codereview.appspot.com/547470044/
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/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 progra
LGTM
https://codereview.appspot.com/573440043/
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 w
> > 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, 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
https://codereview.appspot.com/563420043/diff/547500043/lil
LGTM
https://codereview.appspot.com/563400065/
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
https://codereview.appspot.com/571430048/
LGTM
https://codereview.appspot.com/561350044/
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):
https://codereview.appspot.com/561360043/diff/547520043/Documentation/contributor/quick-start
> 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?
https://cod
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 en
601 - 700 of 722 matches
Mail list logo
