Re: Rewrite Skyline code (issue 547980044 by hanw...@gmail.com)

[dak]

On 2020/04/19 22:35:39, dak wrote:
> On 2020/04/19 21:43:26, hanwenn wrote:
> > (this still needs some work, but the speedup might be worth an early
look)
> 
> * Buildings store Y coordinate of the left edge, rather than the
>   intercept at x==0.0.  This avoid excessive rounding problems when X
>   is large, and lets us compute building.height_at(x[LEFT]) cheaply.
> 
> Since intercept computation order depends on the execution of the
merge
> algorithm and we use floating point arithmetic rather than fixed
point, this
> would make results a lot less reproducible.
> 
> I understand the sentiment and thought about it as well, but it would
only work
> if the _original_ coordinates were retained even after intersection
and
> additional intersections still worked based on the original
coordinates, making
> the merge order (mostly) irrelevant with regard to error propagation.

Actually, one way to keep this pretty order independent would be just
not to intersect until the final pass and use the preceding passes just
for throwing out those line segments that are completely shadowed by
others.  This should not keep all that much more data but would have to
deal with a larger current working set where max/min relations are
checked.  I have no clear view on the expected performance impact.

https://codereview.appspot.com/547980044/

Re: Minor cleanups in stencil-integral.cc (issue 579630043 by hanw...@gmail.com)

[dak]

On 2020/04/24 21:33:12, Carl wrote:
> On 2020/04/24 21:19:57, dak wrote:
> > On 2020/04/24 21:18:12, dak wrote:
> > >
> >
>
https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc
> > > File lily/stencil-integral.cc (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc#newcode465
> > > lily/stencil-integral.cc:465: // more convoluted, but it's fairly
hot path.
> > > Sorry for not being clear: the question was not why this change
was
> effective
> > in
> > > saving time, but why it was valid.  When thickness is zero, you
only update
> > the
> > > upper skyline.  Why would the lower skyline no longer need
updating?
> > 
> > Well, other way round, but apart from that the question stands.
> 
> When thickness is zero, the upper and lower curves are the same. 
Either one
> completes the skyline.

Of course they are the same.  That is not the question.  The question is
why I am considering the identical curve for the minimum skyline while I
don't let it participate in the maximum skyline any more.

If everything is of thickness null, the maximum skyline will be
non-existent while the minimum skyline is there.  While the skylines
should be identical rather than only one of them being there.

https://codereview.appspot.com/579630043/

Re: Minor cleanups in stencil-integral.cc (issue 579630043 by hanw...@gmail.com)

[Carl . D . Sorensen]

On 2020/04/24 21:19:57, dak wrote:
> On 2020/04/24 21:18:12, dak wrote:
> >
>
https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc
> > File lily/stencil-integral.cc (right):
> > 
> >
>
https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc#newcode465
> > lily/stencil-integral.cc:465: // more convoluted, but it's fairly
hot path.
> > Sorry for not being clear: the question was not why this change was
effective
> in
> > saving time, but why it was valid.  When thickness is zero, you only
update
> the
> > upper skyline.  Why would the lower skyline no longer need updating?
> 
> Well, other way round, but apart from that the question stands.

When thickness is zero, the upper and lower curves are the same.  Either
one completes the skyline.


https://codereview.appspot.com/579630043/

Re: Minor cleanups in stencil-integral.cc (issue 579630043 by hanw...@gmail.com)

[dak]

https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):

https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc#newcode465
lily/stencil-integral.cc:465: // more convoluted, but it's fairly hot
path.
Sorry for not being clear: the question was not why this change was
effective in saving time, but why it was valid.  When thickness is zero,
you only update the upper skyline.  Why would the lower skyline no
longer need updating?

https://codereview.appspot.com/579630043/

Re: Minor cleanups in stencil-integral.cc (issue 579630043 by hanw...@gmail.com)

[dak]

On 2020/04/24 21:18:12, dak wrote:
>
https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc
> File lily/stencil-integral.cc (right):
> 
>
https://codereview.appspot.com/579630043/diff/555740043/lily/stencil-integral.cc#newcode465
> lily/stencil-integral.cc:465: // more convoluted, but it's fairly hot
path.
> Sorry for not being clear: the question was not why this change was
effective in
> saving time, but why it was valid.  When thickness is zero, you only
update the
> upper skyline.  Why would the lower skyline no longer need updating?

Well, other way round, but apart from that the question stands.

https://codereview.appspot.com/579630043/

[PATCH v1] Issue 3778: Use bounding box as skylines for markup in svg backend

[Kevin Barry]

As there is no routine for determining skylines for utf-8-string
stencils, they normally fall back to the grob's bounding box, which is
fine. However, when there is a mixture of utf-8-string and other types
of stencil (which have associated skyline functions) in a single grob,
the entire grob gets a skyline determined only from the non-utf-8-string
stencils. This sometimes causes the text portion of such mixed grobs
(e.g.  metronome marks) to collide with other grobs.

While looping over the stencils, check for utf-8-string and if found,
clear the skylines and break out of the loop. Empty skylines forces a
fallback to the grob's bounding box, which restores the behaviour from
before the patch to improve skyline approximations (issue 2148). This
does not fix the issue that there is no routine for determining skylines
for utf-8-strings when the backend is svg, but it does at least remove
the collisions without changing the behaviour in non-broken situations.

Add a suitable (svg backend) regression test.
---
 input/regression/svg-metronome-mark-skylines.ly | 15 +++
 lily/stencil-integral.cc| 10 +-
 2 files changed, 24 insertions(+), 1 deletion(-)
 create mode 100644 input/regression/svg-metronome-mark-skylines.ly

diff --git a/input/regression/svg-metronome-mark-skylines.ly 
b/input/regression/svg-metronome-mark-skylines.ly
new file mode 100644
index 00..844e4a8081
--- /dev/null
+++ b/input/regression/svg-metronome-mark-skylines.ly
@@ -0,0 +1,15 @@
+\header {
+texidoc = "In svg output, a @code{MetronomeMark} or any grob mixing text
+and other glyphs does not collide with other grobs.
+"
+}
+
+\version "2.21.1"
+
+#(ly:set-option 'backend 'svg)
+
+{
+  \tempo "Allegro" 4 = 120
+  r1
+  r4 f''' e''' b''
+}
diff --git a/lily/stencil-integral.cc b/lily/stencil-integral.cc
index 7b12eaf17b..8ac386a10e 100644
--- a/lily/stencil-integral.cc
+++ b/lily/stencil-integral.cc
@@ -1097,7 +1097,15 @@ Stencil::skylines_from_stencil (SCM sten, Real pad, SCM 
rot, Axis a)
   vector boxes;
   vector > buildings;
   for (SCM s = scm_reverse_x (data, SCM_EOL); scm_is_pair (s); s = scm_cdr (s))
-stencil_dispatcher (boxes, buildings, scm_caar (s), scm_cdar (s));
+{
+  // If any stencils are utf-8-string, fall back on the bounding box
+  if (scm_is_eq (scm_cadar (s), ly_symbol2scm ("utf-8-string")))
+{
+  boxes.clear();
+  break;
+}
+  stencil_dispatcher (boxes, buildings, scm_caar (s), scm_cdar (s));
+}
 
   // we use the bounding box if there are no boxes
   // FIXME: Rotation?
-- 
2.25.3

Re: make_draw_bezier_boxes: save work if thickness == 0.0 (issue 551730043 by hanw...@gmail.com)

[Han-Wen Nienhuys]

On Fri, Apr 24, 2020 at 8:56 PM  wrote:
>
>
> https://codereview.appspot.com/551730043/diff/583770043/lily/stencil-integral.cc
> File lily/stencil-integral.cc (right):
>
> https://codereview.appspot.com/551730043/diff/583770043/lily/stencil-integral.cc#newcode496
> lily/stencil-integral.cc:496: break;
> I think that is less readable than desirable.  Instead how about putting
>
> if (th == 0.0)
>break;
>
> behind the push?  That makes very obvious that only one point is being
> pushed without further cleverness.

Yeah, that's better.

> Either way you'll be checking thickness twice if its non-zero.  The
> trivial way to avoid that would be to simply unfold the loop.  Like
>
> points[DOWN].push_back (scm_transform (trans, curve.control_[0] + DOWN *
> normal);
> if (th != 0.0)
>   points[UP].push_back (scm_transform (trans, curve.control_[0] + UP *
> normal;

the UP/DOWN manipulation has been a frequent source of errors in cut &
paste code, so I'd like to keep the loops.

> Written like that, it actually becomes far from trivial to see why this
> optimisation would be valid, so maybe add a comment explaining it for
> the sake of human readers?

The thickness check is cheap. The expensive bit is calculating
directions and bezier points. I added a comment.

-- 
Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen

Re: [PATCH] Issue 3778: Use bounding box as skylines for markup in svg backend

[Kevin Barry]

On Fri, Apr 24, 2020 at 09:25:19PM +0200, Han-Wen Nienhuys wrote:
> Just add a test file under input/regression/ , following the style of
> other files there.

Thanks for the pointer. The only other svg backend file there looks like
it has been deliberately disabled, so apologies if I have done this
wrong. A revised patch will follow with an added regression test.

Kevin

Re: Fix warnings related to po (issue 551830043 by jonas.hahnf...@gmail.com)

[jonas . hahnfeld]

Reviewers: lemzwerg,

Message:
On 2020/04/24 18:25:33, lemzwerg wrote:
> LGTM, thanks a lot!
> 
> https://codereview.appspot.com/551830043/diff/573740043/po/GNUmakefile
> File po/GNUmakefile (right):
> 
>
https://codereview.appspot.com/551830043/diff/573740043/po/GNUmakefile#newcode14
> po/GNUmakefile:14: sed-makestuff = -e
's/[a-zA-Z_/]*make\[[0-9]*\].*//'
> shouldn't this be rather '[0-9]\+'?
> 
>
https://codereview.appspot.com/551830043/diff/573740043/po/GNUmakefile#newcode15
> po/GNUmakefile:15: sed-edstuff = -e 's/[ \.,adic0-9]*//' -e 's/---//'
| sort -u
> I think that within '[...]' you don't have to escape '.'.
> 
> And again I would use '\+' instead of '*'.

To be honest, I have no clue what all of this is doing: I copied it from
stepmake/stepmake/podir-targets.make.

Description:
Fix warnings related to po

Individual commits:
1) Build lilypond-doc in Documentation/po

This avoids warning messages from langdefs.py which tries to load
that locale domain and is also imported during a 'normal' build
while producing the .texi and .info files in Documentation/.

2) Inline po-* targets into po/

Documentation/po/ has a custom po-update.

Please review this at https://codereview.appspot.com/551830043/

Affected files (+35, -57 lines):
  M Documentation/po/GNUmakefile
  M GNUmakefile.in
  M make/lilypond-vars.make
  M po/GNUmakefile
  M stepmake/stepmake/generic-targets.make
  M stepmake/stepmake/podir-targets.make
  M stepmake/stepmake/podir-vars.make


Index: Documentation/po/GNUmakefile
diff --git a/Documentation/po/GNUmakefile b/Documentation/po/GNUmakefile
index 
24dbf6d0580acf952034f2ac9ef0206db664becc..dfdade4e3779b613d5ce107324d6e548b74f220d
 100644
--- a/Documentation/po/GNUmakefile
+++ b/Documentation/po/GNUmakefile
@@ -7,7 +7,6 @@ include $(depth)/make/stepmake.make
 
 doc-localedir=$(outdir)
 LANGS = $(shell $(PYTHON) $(top-src-dir)/python/langdefs.py)
-DOMAIN=lilypond-doc
 DOC_PO_SOURCES = python/auxiliar/postprocess_html.py \
  scripts/auxiliar/tely-gettext.py scripts/auxiliar/translations-status.py
 TELY_FILES = $(shell ls $(depth)/Documentation/*.tely)
@@ -22,7 +21,7 @@ $(outdir)/messages: $(MO_FILES)
for i in $(CATALOGS); do \
  rm -rf $(doc-localedir)/$$i/LC_MESSAGES; \
  mkdir -p $(doc-localedir)/$$i/LC_MESSAGES; \
- $(LN) $(outdir)/$$i.mo $(doc-localedir)/$$i/LC_MESSAGES/$(DOMAIN).mo; 
\
+ $(LN) $(outdir)/$$i.mo 
$(doc-localedir)/$$i/LC_MESSAGES/lilypond-doc.mo; \
done
touch $@
 
@@ -32,10 +31,6 @@ po-update:
cd $(outdir) && xgettext --keyword=_doc -cjn -L Python -o 
buildscripts.pot $(foreach i, $(DOC_PO_SOURCES), $(notdir $(i)))
msgcat -o $(outdir)/doc.pot $(outdir)/buildscripts.pot 
$(outdir)/texi.pot
msgmerge -U lilypond-doc.pot $(outdir)/doc.pot
-   for i in $(CATALOGS); do \
- msgmerge -U $$i.po lilypond-doc.pot; \
+   for i in $(PO_FILES); do \
+ msgmerge -U $$i lilypond-doc.pot; \
done
-
-ifeq ($(out),www)
-local-WWW-1: messages
-endif
Index: GNUmakefile.in
diff --git a/GNUmakefile.in b/GNUmakefile.in
index 
7e40c62db34d7b26e510c5633ef81ef82cf3d42d..67ea83d1979754e98c3ed86b28754ea70430b205
 100644
--- a/GNUmakefile.in
+++ b/GNUmakefile.in
@@ -184,14 +184,10 @@ src-ext = c cc yy ll hh icc py scm tex ps texi itexi tely 
itely sh
 
 doc-clean: snippets-clean $(tree-share-prefix)/lilypond-force
 
-default: $(config_h) build-dir-setup
+default: $(config_h) build-dir-setup doc-messages
 
 build-dir-setup: $(tree-share-prefix)/lilypond-force
 
-PO_FILES = $(call src-wildcard,$(src-depth)/po/*.po)
-HELP_CATALOGS = $(PO_FILES:po/%.po=%)
-CATALOGS = $(HELP_CATALOGS:lilypond=)
-
 # Preparing LilyPond tree for build-dir exec
 link-tree: $(tree-share-prefix)/lilypond-force
 
@@ -230,10 +226,6 @@ $(tree-share-prefix)/lilypond-force: GNUmakefile 
$(outdir)/VERSION
-cd $(tree-share-prefix)/elisp && \
ln -sf ../../../../../elisp/$(outconfbase)/lilypond-words.el . 
&& \
ln -s $(top-src-dir)/elisp/*.el .
-   $(foreach i,$(CATALOGS), \
-   (mkdir -p $(tree-share)/locale/$i/LC_MESSAGES && \
-   cd $(tree-share)/locale/$i/LC_MESSAGES && \
-   ln -sf ../../../../../po/$(outconfbase)/$i.mo lilypond.mo) &&) 
true
touch $(tree-share-prefix)/lilypond-force
 
 link-mf-tree: $(tree-share-prefix)/mf-link-tree
Index: make/lilypond-vars.make
diff --git a/make/lilypond-vars.make b/make/lilypond-vars.make
index 
23d882f7f10034c3c536c5dd4aac4d40b8e82ff1..3ae7ed3db54a3663f9d29c2874800f323b00bfe3
 100644
--- a/make/lilypond-vars.make
+++ b/make/lilypond-vars.make
@@ -81,7 +81,7 @@ export TEXINPUTS
 TEXFONTMAPS=$(top-build-dir)/tex/$(outdir)::
 export TEXFONTMAPS
 
-export LYDOC_LOCALEDIR:= $(top-build-dir)/Documentation/po/out-www
+export LYDOC_LOCALEDIR:= $(top-build-dir)/Documentation/po/out
 
 #texi-html for www only:
 LILYPOND_BOOK_FORMAT=$(if $(subst out-www,,$(notdir $(outdir))),texi,texi-html)
Index: 

Re: [PATCH] Issue 3778: Use bounding box as skylines for markup in svg backend

[Kevin Barry]

On Fri, Apr 24, 2020 at 09:18:10PM +0200, Han-Wen Nienhuys wrote:
> Can you add  a regression test that shows the problem?

Yes. I'll go read the docs to see how to do that and do one up (but feel
free to point me in the right direction...)

Kevin

Re: [PATCH] Issue 3778: Use bounding box as skylines for markup in svg backend

[Han-Wen Nienhuys]

On Fri, Apr 24, 2020 at 9:21 PM Kevin Barry  wrote:
>
> On Fri, Apr 24, 2020 at 09:18:10PM +0200, Han-Wen Nienhuys wrote:
> > Can you add  a regression test that shows the problem?
>
> Yes. I'll go read the docs to see how to do that and do one up (but feel
> free to point me in the right direction...)

Just add a test file under input/regression/ , following the style of
other files there.

-- 
Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen

Re: \mask and \unmaskWithTag

[Dan Eble]

On Apr 23, 2020, at 19:25, Dan Eble  wrote:
> 
>(let ((masked-music (ly:music-property m 'void)))
> (if (and (ly:music? masked-music) (tags-keep-predicate tags))
   ^^
Oops.  This is bogus and the output of the score is correct in spite of it.  
What I intended was to check whether the masked music has the right tags.
— 
Dan

Re: [PATCH] Issue 3778: Use bounding box as skylines for markup in svg backend

[Han-Wen Nienhuys]

Can you add  a regression test that shows the problem?

On Fri, Apr 24, 2020 at 8:50 PM Kevin Barry  wrote:
>
> As there is no routine for determining skylines for utf-8-string
> stencils, they normally fall back to the grob's bounding box, which is
> fine. However, when there is a mixture of utf-8-string and other types
> of stencil (which have associated skyline functions) in a single grob,
> the entire grob gets a skyline determined only from the non-utf-8-string
> stencils. This sometimes causes the text portion of such mixed grobs
> (e.g.  metronome marks) to collide with other grobs.
>
> While looping over the stencils, check for utf-8-string and if found,
> clear the skylines and break out of the loop. Empty skylines forces a
> fallback to the grob's bounding box, which restores the behaviour from
> before the patch to improve skyline approximations (issue 2148). This
> does not fix the issue that there is no routine for determining skylines
> for utf-8-strings when the backend is svg, but it does at least remove
> the collisions without changing the behaviour in non-broken situations.
> ---
>  lily/stencil-integral.cc | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/lily/stencil-integral.cc b/lily/stencil-integral.cc
> index 7b12eaf17b..47179148cd 100644
> --- a/lily/stencil-integral.cc
> +++ b/lily/stencil-integral.cc
> @@ -1097,7 +1097,14 @@ Stencil::skylines_from_stencil (SCM sten, Real pad, 
> SCM rot, Axis a)
>vector boxes;
>vector > buildings;
>for (SCM s = scm_reverse_x (data, SCM_EOL); scm_is_pair (s); s = scm_cdr 
> (s))
> -stencil_dispatcher (boxes, buildings, scm_caar (s), scm_cdar (s));
> +{
> +  // If any stencils are utf-8-string, fall back on the bounding box
> +  if (scm_is_eq (scm_cadar (s), ly_symbol2scm ("utf-8-string"))) {
> +  boxes.clear();
> +  break;
> +  }
> +  stencil_dispatcher (boxes, buildings, scm_caar (s), scm_cdar (s));
> +}
>
>// we use the bounding box if there are no boxes
>// FIXME: Rotation?
> --
> 2.25.3
>
>


-- 
Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen

Re: make_draw_bezier_boxes: save work if thickness == 0.0 (issue 551730043 by hanw...@gmail.com)

[dak]

https://codereview.appspot.com/551730043/diff/583770043/lily/stencil-integral.cc
File lily/stencil-integral.cc (right):

https://codereview.appspot.com/551730043/diff/583770043/lily/stencil-integral.cc#newcode496
lily/stencil-integral.cc:496: break;
I think that is less readable than desirable.  Instead how about putting

if (th == 0.0)
   break;

behind the push?  That makes very obvious that only one point is being
pushed without further cleverness.

Either way you'll be checking thickness twice if its non-zero.  The
trivial way to avoid that would be to simply unfold the loop.  Like

points[DOWN].push_back (scm_transform (trans, curve.control_[0] + DOWN *
normal);
if (th != 0.0)
  points[UP].push_back (scm_transform (trans, curve.control_[0] + UP *
normal;

Written like that, it actually becomes far from trivial to see why this
optimisation would be valid, so maybe add a comment explaining it for
the sake of human readers?

https://codereview.appspot.com/551730043/

Re: Rewrite Skyline code (issue 547980044 by hanw...@gmail.com)

[hanwenn]

https://codereview.appspot.com/547980044/diff/567490043/lily/include/skyline.hh
File lily/include/skyline.hh (right):

https://codereview.appspot.com/547980044/diff/567490043/lily/include/skyline.hh#newcode152
lily/include/skyline.hh:152: }
On 2020/04/24 17:12:48, hanwenn wrote:
> On 2020/04/24 16:33:04, hahnjo wrote:
> > Why do you need all of this in the header file? As far as I can see,
nobody
> else
> > is calling these methods, so the argument of inlinining does not
apply.
> 
> trimmed this a bit.

looks like GCC doesn't want to inline the functions if I do that,
leading to significant slowdown.

https://codereview.appspot.com/547980044/

Re: Extracting approximate outlines from FT ?

[Han-Wen Nienhuys]

On Fri, Apr 24, 2020 at 5:54 PM Han-Wen Nienhuys  wrote:
>
> On Fri, Apr 17, 2020 at 7:56 PM Werner LEMBERG  wrote:
> > > I've been looking into performance, and one big source of slowness
> > > is calculating skylines. I found that we calculate exact skylines
> > > based on glyph shapes for some symbols.
> > >
> > > For example, for the F clef, we compute an outline of 23 and 20
> > > segments, based on sampling the exact bezier curves in the glyph.
> > > This is silly (because the F clef is obscured by the staff symbol),
> > > but even if it weren't, it seems extremely expensive overkill.  Can
> > > you recommend a mechanism by which we can get a linear approximation
> > > of the curve directly out of freetype?
> >
> > FreeType doesn't offer this, sorry – at least not in a form that would
> > be usable for LilyPond IMHO.  You could use `FT_Outline_Decompose` and
> > register your own handler for simplifying Bezier arcs – given the
> > well-behaving of font outlines (at least in Feta), maybe the direct
> > use of control points are sufficient, possibly doing one or at most
> > two de Casteljau subdivisions?
>
> Good idea. I'll keep it in mind. Other question: is there a standard
> wrt to outline direction? If I can assume that external outlines go in
> clockwise order, I could skip left-pointing path elements when
> creating the upwards skyline, cutting the amount of work in half.
> Also, is there is way to detect internal curves (eg. the inner curve
> of the O glyph?).

In https://docs.microsoft.com/en-us/typography/opentype/spec/ttch01 it
is explained that outside curves go clockwise. Does that hold for
other font types too? (PFA, OTF?)

-- 
Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen

[PATCH] Issue 3778: Use bounding box as skylines for markup in svg backend

[Kevin Barry]

As there is no routine for determining skylines for utf-8-string
stencils, they normally fall back to the grob's bounding box, which is
fine. However, when there is a mixture of utf-8-string and other types
of stencil (which have associated skyline functions) in a single grob,
the entire grob gets a skyline determined only from the non-utf-8-string
stencils. This sometimes causes the text portion of such mixed grobs
(e.g.  metronome marks) to collide with other grobs.

While looping over the stencils, check for utf-8-string and if found,
clear the skylines and break out of the loop. Empty skylines forces a
fallback to the grob's bounding box, which restores the behaviour from
before the patch to improve skyline approximations (issue 2148). This
does not fix the issue that there is no routine for determining skylines
for utf-8-strings when the backend is svg, but it does at least remove
the collisions without changing the behaviour in non-broken situations.
---
 lily/stencil-integral.cc | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lily/stencil-integral.cc b/lily/stencil-integral.cc
index 7b12eaf17b..47179148cd 100644
--- a/lily/stencil-integral.cc
+++ b/lily/stencil-integral.cc
@@ -1097,7 +1097,14 @@ Stencil::skylines_from_stencil (SCM sten, Real pad, SCM 
rot, Axis a)
   vector boxes;
   vector > buildings;
   for (SCM s = scm_reverse_x (data, SCM_EOL); scm_is_pair (s); s = scm_cdr (s))
-stencil_dispatcher (boxes, buildings, scm_caar (s), scm_cdar (s));
+{
+  // If any stencils are utf-8-string, fall back on the bounding box
+  if (scm_is_eq (scm_cadar (s), ly_symbol2scm ("utf-8-string"))) {
+  boxes.clear();
+  break;
+  }
+  stencil_dispatcher (boxes, buildings, scm_caar (s), scm_cdar (s));
+}
 
   // we use the bounding box if there are no boxes
   // FIXME: Rotation?
-- 
2.25.3

Re: make_draw_bezier_boxes: save work if thickness == 0.0 (issue 551730043 by hanw...@gmail.com)

[hanwenn]

Reviewers: Dan Eble,

Message:
The percentages between different have looked fairly repeatable, but as
a less invasive measure is to just tweak the final 'if'

Description:
make_draw_bezier_boxes: save work if thickness == 0.0

This drop make_draw_bezier_boxes from 0.73% to 0.44% in the profile for
MSDM

Please review this at https://codereview.appspot.com/551730043/

Affected files (+24, -5 lines):
  M lily/stencil-integral.cc


Index: lily/stencil-integral.cc
diff --git a/lily/stencil-integral.cc b/lily/stencil-integral.cc
index 
189cdcdc881ec0c1c417a4a783b4675446ec4207..e43bbee6cd14756bcf0b6e4a575c68b3abd4a136
 100644
--- a/lily/stencil-integral.cc
+++ b/lily/stencil-integral.cc
@@ -480,12 +480,20 @@ make_draw_bezier_boxes (vector ,
+ (temp3 - temp2).length ())
   / QUANTIZATION_UNIT);
 
-  Offset d0 = curve.dir_at_point (0.0);
-  Offset d1 = curve.dir_at_point (1.0);
+  Offset d0;
+  Offset d1;
+
+  Offset normal;
+  if (th > 0)
+{
+  d0 = curve.dir_at_point (0.0);
+  normal = get_normal ((th / 2) * d0);
+}
 
-  Offset normal = get_normal ((th / 2) * d0);
   for (DOWN_and_UP (d))
 {
+  if (th == 0.0 && d == UP)
+break;
   points[d].push_back (
 scm_transform (trans, curve.control_[0] + d * normal));
 }
@@ -497,14 +505,23 @@ make_draw_bezier_boxes (vector ,
 
   for (DOWN_and_UP (d))
 {
+  if (th == 0.0 && d == UP)
+break;
   points[d].push_back (
 scm_transform (trans, curve.curve_point (pt) + d * norm));
 }
 }
 
-  normal = get_normal ((th / 2) * d1);
+  if (th > 0)
+{
+  d1 = curve.dir_at_point (1.0);
+  normal = get_normal ((th / 2) * d1);
+}
+
   for (DOWN_and_UP (d))
 {
+  if (th == 0.0 && d == UP)
+break;
   points[d].push_back (
 scm_transform (trans, curve.control_[3] + d * normal));
 }
@@ -514,13 +531,15 @@ make_draw_bezier_boxes (vector ,
   Box b;
   for (DOWN_and_UP (d))
 {
+  if (th == 0.0 && d == UP)
+break;
   b.add_point (points[d][i]);
   b.add_point (points[d][i + 1]);
 }
   boxes.push_back (b);
 }
 
-  if (th >= 0)
+  if (th > 0)
 {
   // beg line cap
   create_path_cap (boxes, buildings, trans, curve.control_[0], th / 2, 
-d0);

make_draw_bezier_boxes: save work if thickness == 0.0 (issue 551730043 by hanw...@gmail.com)

[nine . fierce . ballads]

I see I'm late to the party with this comment, but what are the error
bars on that 0.29%?  I'm not sure the decreased readability is worth it.

https://codereview.appspot.com/551730043/

Fix warnings related to po (issue 551830043 by jonas.hahnf...@gmail.com)

[lemzwerg--- via Discussions on LilyPond development]

LGTM, thanks a lot!


https://codereview.appspot.com/551830043/diff/573740043/po/GNUmakefile
File po/GNUmakefile (right):

https://codereview.appspot.com/551830043/diff/573740043/po/GNUmakefile#newcode14
po/GNUmakefile:14: sed-makestuff = -e 's/[a-zA-Z_/]*make\[[0-9]*\].*//'
shouldn't this be rather '[0-9]\+'?

https://codereview.appspot.com/551830043/diff/573740043/po/GNUmakefile#newcode15
po/GNUmakefile:15: sed-edstuff = -e 's/[ \.,adic0-9]*//' -e 's/---//' |
sort -u
I think that within '[...]' you don't have to escape '.'.

And again I would use '\+' instead of '*'.

https://codereview.appspot.com/551830043/

Re: Rewrite Skyline code (issue 547980044 by hanw...@gmail.com)

[hanwenn]

On 2020/04/24 17:12:48, hanwenn wrote:
>
https://codereview.appspot.com/547980044/diff/567490043/lily/include/skyline.hh
> File lily/include/skyline.hh (right):
> 
>
https://codereview.appspot.com/547980044/diff/567490043/lily/include/skyline.hh#newcode152
> lily/include/skyline.hh:152: }
> On 2020/04/24 16:33:04, hahnjo wrote:
> > Why do you need all of this in the header file? As far as I can see,
nobody
> else
> > is calling these methods, so the argument of inlinining does not
apply.
> 
> trimmed this a bit.
> 
>
https://codereview.appspot.com/547980044/diff/567490043/lily/skyline-pair.cc
> File lily/skyline-pair.cc (left):
> 
>
https://codereview.appspot.com/547980044/diff/567490043/lily/skyline-pair.cc#oldcode100
> lily/skyline-pair.cc:100: Skyline_pair::print_points () const
> On 2020/04/24 16:33:04, hahnjo wrote:
> > You should not delete a function without removing the declaration in
> > lily/include/skyline-pair.hh. Also this seems unrelated to the
performance
> > improvements.
> 
> the print() function prints the building in terms of y-intercept at
x=0.0 +
> slope. Since we don't store the y-intercept anymore, that is useless
now. The
> points are also more convenient for debugging, which is the only
objective of
> this function.
> 
>
https://codereview.appspot.com/547980044/diff/567490043/lily/skyline.cc
> File lily/skyline.cc (right):
> 
>
https://codereview.appspot.com/547980044/diff/567490043/lily/skyline.cc#newcode780
> lily/skyline.cc:780: Skyline::to_segments (Axis horizon_axis) const
> On 2020/04/24 16:33:04, hahnjo wrote:
> > What's the advantage of this code over the old method?
> 
> the buildings making up the skylines have become non-contiguous, so
you can't
> simply connect all the points any more.  (If you'd do that, there
would be
> diagonal line segments printed in debug output that aren't really
there.)
> 
>
https://codereview.appspot.com/547980044/diff/567490043/scm/define-grobs.scm
> File scm/define-grobs.scm (right):
> 
>
https://codereview.appspot.com/547980044/diff/567490043/scm/define-grobs.scm#newcode1657
> scm/define-grobs.scm:1657: (stencil . ,ly:paper-column::print)
> On 2020/04/24 16:33:04, hahnjo wrote:
> > Is this change intentional? And intentionally in this review?
> 
> thanks, no. I was already wondering why the cell count shot up in the
profile
> :-)

grmbl.- and now the speed difference is gone again.  I hate my
autoscaling CPU.


https://codereview.appspot.com/547980044/

Re: Rewrite Skyline code (issue 547980044 by hanw...@gmail.com)

[hanwenn]

https://codereview.appspot.com/547980044/diff/567490043/lily/include/skyline.hh
File lily/include/skyline.hh (right):

https://codereview.appspot.com/547980044/diff/567490043/lily/include/skyline.hh#newcode152
lily/include/skyline.hh:152: }
On 2020/04/24 16:33:04, hahnjo wrote:
> Why do you need all of this in the header file? As far as I can see,
nobody else
> is calling these methods, so the argument of inlinining does not
apply.

trimmed this a bit.

https://codereview.appspot.com/547980044/diff/567490043/lily/skyline-pair.cc
File lily/skyline-pair.cc (left):

https://codereview.appspot.com/547980044/diff/567490043/lily/skyline-pair.cc#oldcode100
lily/skyline-pair.cc:100: Skyline_pair::print_points () const
On 2020/04/24 16:33:04, hahnjo wrote:
> You should not delete a function without removing the declaration in
> lily/include/skyline-pair.hh. Also this seems unrelated to the
performance
> improvements.

the print() function prints the building in terms of y-intercept at
x=0.0 + slope. Since we don't store the y-intercept anymore, that is
useless now. The points are also more convenient for debugging, which is
the only objective of this function.

https://codereview.appspot.com/547980044/diff/567490043/lily/skyline.cc
File lily/skyline.cc (right):

https://codereview.appspot.com/547980044/diff/567490043/lily/skyline.cc#newcode780
lily/skyline.cc:780: Skyline::to_segments (Axis horizon_axis) const
On 2020/04/24 16:33:04, hahnjo wrote:
> What's the advantage of this code over the old method?

the buildings making up the skylines have become non-contiguous, so you
can't simply connect all the points any more.  (If you'd do that, there
would be diagonal line segments printed in debug output that aren't
really there.)

https://codereview.appspot.com/547980044/diff/567490043/scm/define-grobs.scm
File scm/define-grobs.scm (right):

https://codereview.appspot.com/547980044/diff/567490043/scm/define-grobs.scm#newcode1657
scm/define-grobs.scm:1657: (stencil . ,ly:paper-column::print)
On 2020/04/24 16:33:04, hahnjo wrote:
> Is this change intentional? And intentionally in this review?

thanks, no. I was already wondering why the cell count shot up in the
profile :-)

https://codereview.appspot.com/547980044/

Re: Rewrite Skyline code (issue 547980044 by hanw...@gmail.com)

[jonas . hahnfeld]

https://codereview.appspot.com/547980044/diff/567490043/lily/include/skyline.hh
File lily/include/skyline.hh (right):

https://codereview.appspot.com/547980044/diff/567490043/lily/include/skyline.hh#newcode152
lily/include/skyline.hh:152: }
Why do you need all of this in the header file? As far as I can see,
nobody else is calling these methods, so the argument of inlinining does
not apply.

https://codereview.appspot.com/547980044/diff/567490043/lily/skyline-pair.cc
File lily/skyline-pair.cc (left):

https://codereview.appspot.com/547980044/diff/567490043/lily/skyline-pair.cc#oldcode100
lily/skyline-pair.cc:100: Skyline_pair::print_points () const
You should not delete a function without removing the declaration in
lily/include/skyline-pair.hh. Also this seems unrelated to the
performance improvements.

https://codereview.appspot.com/547980044/diff/567490043/lily/skyline.cc
File lily/skyline.cc (right):

https://codereview.appspot.com/547980044/diff/567490043/lily/skyline.cc#newcode780
lily/skyline.cc:780: Skyline::to_segments (Axis horizon_axis) const
What's the advantage of this code over the old method?

https://codereview.appspot.com/547980044/diff/567490043/scm/define-grobs.scm
File scm/define-grobs.scm (right):

https://codereview.appspot.com/547980044/diff/567490043/scm/define-grobs.scm#newcode1657
scm/define-grobs.scm:1657: (stencil . ,ly:paper-column::print)
Is this change intentional? And intentionally in this review?

https://codereview.appspot.com/547980044/

Re: Extracting approximate outlines from FT ?

[Han-Wen Nienhuys]

On Fri, Apr 17, 2020 at 7:56 PM Werner LEMBERG  wrote:
> > I've been looking into performance, and one big source of slowness
> > is calculating skylines. I found that we calculate exact skylines
> > based on glyph shapes for some symbols.
> >
> > For example, for the F clef, we compute an outline of 23 and 20
> > segments, based on sampling the exact bezier curves in the glyph.
> > This is silly (because the F clef is obscured by the staff symbol),
> > but even if it weren't, it seems extremely expensive overkill.  Can
> > you recommend a mechanism by which we can get a linear approximation
> > of the curve directly out of freetype?
>
> FreeType doesn't offer this, sorry – at least not in a form that would
> be usable for LilyPond IMHO.  You could use `FT_Outline_Decompose` and
> register your own handler for simplifying Bezier arcs – given the
> well-behaving of font outlines (at least in Feta), maybe the direct
> use of control points are sufficient, possibly doing one or at most
> two de Casteljau subdivisions?

Good idea. I'll keep it in mind. Other question: is there a standard
wrt to outline direction? If I can assume that external outlines go in
clockwise order, I could skip left-pointing path elements when
creating the upwards skyline, cutting the amount of work in half.
Also, is there is way to detect internal curves (eg. the inner curve
of the O glyph?).

-- 
Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen

Re: Annoying 'langdefs.py' warning

[Jonas Hahnfeld]

Am Sonntag, den 19.04.2020, 17:22 +0200 schrieb Werner LEMBERG:
> >> Is there a way to suppress them?
> > 
> > Well, submitting fixes?
> 
> I looked into both issues some time ago, but couldn't find out a good
> solution.  Thanks to your clean-ups I can imagine that it's now easier
> to handle both problems.
> 
> > IIRC langdefs.py complains that we set LANG=C in the build
> > process. Not really sure what the correct solution is, could be as
> > easy as removing the warning (don't scream at me if not...)
> 
> :-)
> 
> > [...] If nobody objects, I can probably prepare a patch (obvious
> > caveat: take care of values used from
> > stepmake/stepmake/podir-vars.make).
> 
> Great, and thanks in advance!

https://sourceforge.net/p/testlilyissues/issues/5932/
https://codereview.appspot.com/551830043

I hope I covered all existing use cases that should continue to be
live:
 - make po-update and po-replace work from top-level
 - make -C Documentation/po/ po-update does *something*

However I'm pretty sure that many translations in the latter are dead:
As far as I understand we don't translate the docs by putting the
strings in Documentation/po, but rather by copying the files and
translating them below Documentation/lang. Still, translations for the
footer in HTML pages appear to be used (like "Other languages:" and
"About automatic language selection"). Whoever is interested in
improving this situation is more than welcome!

Jonas


signature.asc
Description: This is a digitally signed message part

Re: Optimize get_path_list. (issue 579570043 by hanw...@gmail.com)

[hanwenn]

On 2020/04/13 19:11:22, hanwenn wrote:
>
https://codereview.appspot.com/579570043/diff/577750045/lily/stencil-integral.cc
> File lily/stencil-integral.cc (right):
> 
>
https://codereview.appspot.com/579570043/diff/577750045/lily/stencil-integral.cc#newcode115
> lily/stencil-integral.cc:115: && (SCM_EQ_P (head, ly_symbol2scm
("moveto"))
> On 2020/04/13 17:50:33, dak wrote:
> > SCM_EQ_P is an old deprecated macro.  Use scm_is_eq instead.  Same
> performance.
> 
> Done.
> 
>
https://codereview.appspot.com/579570043/diff/577750045/lily/stencil-integral.cc#newcode127
> lily/stencil-integral.cc:127: return get_path_list (scm_cdr (l));
> On 2020/04/13 17:50:33, dak wrote:
> > Not caused by this patch, but this does look like something nicer
addressed by
> a
> > loop rather than tail recursion with regard to C++ idioms.
> 
> I'm gonna pass on this for now.

commit d48d79b52c28cf9c82d088b2fdf8edbf452d6f94
Author: Han-Wen Nienhuys 
Date:   Mon Apr 13 18:36:46 2020 +0200

Optimize get_path_list.



https://codereview.appspot.com/579570043/

Re: Use vectors rather than lists for skylines. (issue 583750043 by hanw...@gmail.com)

[David Kastrup]

Han-Wen Nienhuys  writes:

> On Fri, Apr 24, 2020 at 3:15 PM David Kastrup  wrote:
>>
>
>> > If you want me to use "auto" instead of
>> > "vector::const_iterator", can you just point to the line in
>> > the code and say "could you use auto instead of an iterator here"?
>>
>> Wouldn't "everywhere" be sort of obvious?
>
> yes, it would. However when you talk about fragmentation, or your
> experiments about alternative merge algorithms, I get the impression
> you are not concerned with the actual code in the patch. So that is
> why I ask you to point to the code.

I can address several issues in a single mail.

-- 
David Kastrup

Re: Use vectors rather than lists for skylines. (issue 583750043 by hanw...@gmail.com)

[Han-Wen Nienhuys]

On Fri, Apr 24, 2020 at 3:15 PM David Kastrup  wrote:
>

> > If you want me to use "auto" instead of
> > "vector::const_iterator", can you just point to the line in
> > the code and say "could you use auto instead of an iterator here"?
>
> Wouldn't "everywhere" be sort of obvious?

yes, it would. However when you talk about fragmentation, or your
experiments about alternative merge algorithms, I get the impression
you are not concerned with the actual code in the patch. So that is
why I ask you to point to the code.

-- 
Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen

Re: Use vectors rather than lists for skylines. (issue 583750043 by hanw...@gmail.com)

[jonas . hahnfeld]

On 2020/04/24 13:09:53, hanwenn wrote:
> On Fri, Apr 24, 2020 at 3:04 PM 
wrote:
> >
> > Han-Wen, any reason not to use range-based loops in the places I
pointed
> > to in the last patch set?
> 
> I'm rewriting a lot of this code anyway in
> https://codereview.appspot.com/547980044/ so changing it to range
> loops involves more wasted editing. I can do it if you care about it,
> though.

Oh right, I forgot. Either way is fine for me, I guess you'll get
conflicts with both.

https://codereview.appspot.com/583750043/

Re: Use vectors rather than lists for skylines. (issue 583750043 by hanw...@gmail.com)

[David Kastrup]

hanw...@gmail.com writes:

> Here is how I have experienced this discussion:
>
> DAK: this is micro-optimization that causes memory fragmentation.
>
> HW: No, it's not; here is a benchmark that shows decreased memory use as
> well.
>
> DAK: We could use even different data structures in the future.
>
> HW: This seems very philosophical to me.

It is interesting that you call it "philosophical" to want to change
data structures while getting a patch of yours reviewed that wants to
change data structures.  In particular after I indicated that I even
have code.

Why is it "philosophical" when someone other than you does it?

> Maybe I misunderstood, because the hardcoding of a specific STL
> container type predates this patch.

Changing the used container type would be a reasonable point of time for
changing the hardcoding, particularly since it facilitates comparisons.
Also C++11 makes this considerably less awkward than previously, and
code written with lists in mind tends to require less adaptation to use
via vectors than the other way round, anyway.

It's not just the time that a computer may save when going through the
code that is important.  Humans are worth consideration as well.

> If you want me to use "auto" instead of
> "vector::const_iterator", can you just point to the line in
> the code and say "could you use auto instead of an iterator here"?

Wouldn't "everywhere" be sort of obvious?

> https://codereview.appspot.com/583750043/

-- 
David Kastrup

Re: Use vectors rather than lists for skylines. (issue 583750043 by hanw...@gmail.com)

[Han-Wen Nienhuys]

On Fri, Apr 24, 2020 at 3:04 PM  wrote:
>
> Han-Wen, any reason not to use range-based loops in the places I pointed
> to in the last patch set?

I'm rewriting a lot of this code anyway in
https://codereview.appspot.com/547980044/ so changing it to range
loops involves more wasted editing. I can do it if you care about it,
though.



-- 
Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen

Re: Use vectors rather than lists for skylines. (issue 583750043 by hanw...@gmail.com)

[jonas . hahnfeld]

Han-Wen, any reason not to use range-based loops in the places I pointed
to in the last patch set?


https://codereview.appspot.com/583750043/diff/545930043/lily/skyline.cc
File lily/skyline.cc (right):

https://codereview.appspot.com/583750043/diff/545930043/lily/skyline.cc#newcode184
lily/skyline.cc:184: for (vector::iterator i =
buildings_.begin ();
auto

https://codereview.appspot.com/583750043/diff/545930043/lily/skyline.cc#newcode753
lily/skyline.cc:753: for (vector::const_reverse_iterator i
(buildings_.rbegin ());
auto or rather range-based for

https://codereview.appspot.com/583750043/

Re: Use vectors rather than lists for skylines. (issue 572040043 by hanw...@gmail.com)

[hanwenn]

mixup, sorry.

https://codereview.appspot.com/572040043/

Re: Use vectors rather than lists for skylines. (issue 583750043 by hanw...@gmail.com)

[hanwenn]

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc
File lily/skyline.cc (right):

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode183
lily/skyline.cc:183: vector::iterator i;
On 2020/04/24 07:08:50, hahnjo wrote:
> move into for loop and make auto

Done.

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode189
lily/skyline.cc:189: vector::iterator last = i;
On 2020/04/24 07:08:50, hahnjo wrote:
> auto

Done.

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode216
lily/skyline.cc:216: vector::const_iterator cit = scp->begin
();
On 2020/04/24 07:08:51, hahnjo wrote:
> auto (2x)

Done.

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode319
lily/skyline.cc:319: for (vector::const_iterator i =
buildings.begin ();
On 2020/04/24 07:08:51, hahnjo wrote:
> auto or range-based for

Done.

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode565
lily/skyline.cc:565: vector::iterator end = buildings_.end ();
On 2020/04/24 07:08:50, hahnjo wrote:
> auto or range-based for

Done.

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode574
lily/skyline.cc:574: for (vector::iterator i =
buildings_.begin (); i != end; i++)
On 2020/04/24 07:08:51, hahnjo wrote:
> auto or range-based for

Done.

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode620
lily/skyline.cc:620: for (vector::const_iterator i =
buildings_.begin ();
On 2020/04/24 07:08:51, hahnjo wrote:
> auto or range-based for

Done.

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode677
lily/skyline.cc:677: vector::const_iterator j =
other.buildings_.begin ();
On 2020/04/24 07:08:51, hahnjo wrote:
> auto (2x)

Done.

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode711
lily/skyline.cc:711: vector::const_iterator i;
On 2020/04/24 07:08:51, hahnjo wrote:
> move into for and auto or range-based for

Done.

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode727
lily/skyline.cc:727: vector::const_iterator i;
On 2020/04/24 07:08:50, hahnjo wrote:
> move into for and auto or range-based for

Done.

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode748
lily/skyline.cc:748: for (vector::const_iterator i
(buildings_.begin ());
On 2020/04/24 07:08:51, hahnjo wrote:
> auto or range-based for

Done.

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode760
lily/skyline.cc:760: for (vector::const_reverse_iterator i
(buildings_.rbegin ());
On 2020/04/24 07:08:51, hahnjo wrote:
> auto or range-based for

Done.

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode792
lily/skyline.cc:792: for (vector::const_iterator i
(buildings_.begin ());
On 2020/04/24 07:08:51, hahnjo wrote:
> auto or range-based for

Done.

https://codereview.appspot.com/583750043/

Re: Use vectors rather than lists for skylines. (issue 583750043 by hanw...@gmail.com)

[hanwenn]

Here is how I have experienced this discussion:

DAK: this is micro-optimization that causes memory fragmentation.

HW: No, it's not; here is a benchmark that shows decreased memory use as
well.

DAK: We could use even different data structures in the future.

HW: This seems very philosophical to me.


Maybe I misunderstood, because the hardcoding of a specific STL
container type predates this patch.

If you want me to use "auto" instead of
"vector::const_iterator", can you just point to the line in
the code and say "could you use auto instead of an iterator here"?

thanks,

https://codereview.appspot.com/583750043/

Re: Verifying fixes?

[James Lowe]

On 23/04/2020 18:32, Valentin Villenave wrote:

On Thursday, April 23, 2020, Carl Sorensen  wrote:.

Do you believe we should Verify the fixes, or should we just leave the
issues closed without verification?  I'm willing to spend some time doing
the verification, if we think it's valuable to do so.


I'm available to help with that as well. (Yay, only 230 issues each! Unless
Simon, Harm, James or anyone else wants to join in? :-)

V.


I will be available to help with this if needed on Saturday/Sunday.

I assume that the CG documented steps are all that I need to refer to?

James

Re: Refactor get/set_property to take the item as first argument (issue 573670043 by d...@gnu.org)

[jonas . hahnfeld]

On 2020/04/24 08:15:09, Valentin Villenave wrote:
> On 2020/04/24 07:20:55, hahnjo wrote:
> > If there are no technical objections, I think we should move
> > forward and let something as big sit around for too long.
> 
> Just for clarification, did you miss a "not" in that sentence?

Ehm yes. Probably past me couldn't decide where to put it...

If there are no technical objections, I think we should move
forward and not let something as big sit around for too long.

https://codereview.appspot.com/573670043/

Re: Refactor get/set_property to take the item as first argument (issue 573670043 by d...@gnu.org)

[v . villenave]

On 2020/04/24 07:20:55, hahnjo wrote:
> If there are no technical objections, I think we should move
> forward and let something as big sit around for too long.

Just for clarification, did you miss a "not" in that sentence?

Cheers,
  - V.

https://codereview.appspot.com/573670043/

Re: Make doc currently broken

[Valentin Villenave]

On 4/23/20, Federico Bruni  wrote:
> I confirm it's a stale dependency file.
> I had the same problem while testing my patch before pushing and IIRC
> removing Documentation/it/out-www/web.texi fixed it.

OK, thanks! I should have thought about doc-clean.

Cheers,
  - V.

Re: Refactor get/set_property to take the item as first argument (issue 573670043 by d...@gnu.org)

[jonas . hahnfeld]

So I've largely kept out of the discussion for now, mostly because I'm
not overly familiar with the code. However I fully agree with Dan that a
macro pretending to be a member function is neither obvious nor C++
style. As such I'm in favor of doing the conversion as it better meets
the expectations of average developers (at least mine).

I also agree with David that this patch doesn't prescribe any future
direction, which seems to be Han-Wen's fear. Any possible follow-up is
subject to the same review process. If there are no technical
objections, I think we should move forward and let something as big sit
around for too long.

https://codereview.appspot.com/573670043/

Re: Use vectors rather than lists for skylines. (issue 583750043 by hanw...@gmail.com)

[jonas . hahnfeld]

I generally agree with David that having the code agnostic of the
container would be an improvement. This doesn't need to be a typedef yet
as reserve() is unique to std::vector (at least not in std::list) and
makes sense in the places it is used here. A rather obvious change (that
I'm very surprised you didn't do in the first place) is replacing all
iterator types by auto. Neither the code nor the reader should care what
the type is, and it is long enough to considerably clutter the code.
Even better, just use range-based loops, that should be standard C++
style by now.
Also the patch is currently very hard to read because it still has the
Interval changes that should be in master already (so it probably
doesn't even apply).

What I'm not happy about is changing the patch status based on saying
it's a "philosphical discussion". Every comment should be considered
important, and it has happened often enough that something induces the
submitter's creativity that might sound philosophical at first.


https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc
File lily/skyline.cc (right):

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode183
lily/skyline.cc:183: vector::iterator i;
move into for loop and make auto

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode189
lily/skyline.cc:189: vector::iterator last = i;
auto

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode216
lily/skyline.cc:216: vector::const_iterator cit = scp->begin
();
auto (2x)

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode319
lily/skyline.cc:319: for (vector::const_iterator i =
buildings.begin ();
auto or range-based for

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode565
lily/skyline.cc:565: vector::iterator end = buildings_.end ();
auto or range-based for

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode574
lily/skyline.cc:574: for (vector::iterator i =
buildings_.begin (); i != end; i++)
auto or range-based for

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode620
lily/skyline.cc:620: for (vector::const_iterator i =
buildings_.begin ();
auto or range-based for

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode677
lily/skyline.cc:677: vector::const_iterator j =
other.buildings_.begin ();
auto (2x)

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode711
lily/skyline.cc:711: vector::const_iterator i;
move into for and auto or range-based for

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode727
lily/skyline.cc:727: vector::const_iterator i;
move into for and auto or range-based for

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode748
lily/skyline.cc:748: for (vector::const_iterator i
(buildings_.begin ());
auto or range-based for

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode760
lily/skyline.cc:760: for (vector::const_reverse_iterator i
(buildings_.rbegin ());
auto or range-based for

https://codereview.appspot.com/583750043/diff/577780043/lily/skyline.cc#newcode792
lily/skyline.cc:792: for (vector::const_iterator i
(buildings_.begin ());
auto or range-based for

https://codereview.appspot.com/583750043/