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

Issue 555420043: Fix most encoding problems with Guile 2.x (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months ago by hahnjo
Modified:
2 months, 2 weeks ago
Reviewers:
hanwenn
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Fix most encoding problems with Guile 2.x Individual commits: 1) Treat possibly incomplete UTF-8 as binary replace_special_characters checks that the substring doesn't start mid-UTF-8, but it does not guarantee that it ends in a complete glyph. So just explicitly treat it as binary when creating the SCM. While modifying the function, avoid comparison of zero-length substrings. 2) Use UTF-8 for all conversions to / from Scheme LilyPond really expects all input to be encoded in UTF-8, and we should not let GUILE 2.x mangle with it.

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -8 lines) Patch
M lily/general-scheme.cc View 1 chunk +3 lines, -1 line 0 comments Download
M lily/include/lily-guile-macros.hh View 1 chunk +2 lines, -0 lines 0 comments Download
M lily/lily-guile.cc View 2 chunks +2 lines, -3 lines 0 comments Download
M lily/text-interface.cc View 1 chunk +9 lines, -4 lines 1 comment Download

Messages

Total messages: 5
hahnjo
FYI: This makes a `check` with Guile 2.2 for `baseline` from Guile 1.8 almost clean. ...
4 months ago (2020-03-07 21:47:31 UTC) #1
hanwenn
LGTM maybe add a TODO for the clumsy thing. https://codereview.appspot.com/555420043/diff/549710049/lily/text-interface.cc File lily/text-interface.cc (right): https://codereview.appspot.com/555420043/diff/549710049/lily/text-interface.cc#newcode70 lily/text-interface.cc:70: ...
4 months ago (2020-03-07 23:15:09 UTC) #2
hahnjo
On 2020/03/07 23:15:09, hanwenn wrote: > https://codereview.appspot.com/555420043/diff/549710049/lily/text-interface.cc > File lily/text-interface.cc (right): > > https://codereview.appspot.com/555420043/diff/549710049/lily/text-interface.cc#newcode70 > ...
4 months ago (2020-03-08 09:33:24 UTC) #3
hanwenn
On 2020/03/08 09:33:24, hahnjo wrote: > On 2020/03/07 23:15:09, hanwenn wrote: > > https://codereview.appspot.com/555420043/diff/549710049/lily/text-interface.cc > ...
4 months ago (2020-03-08 09:37:23 UTC) #4
hahnjo
4 months ago (2020-03-08 09:42:29 UTC) #5
On 2020/03/08 09:37:23, hanwenn wrote:
> On 2020/03/08 09:33:24, hahnjo wrote:
> > On 2020/03/07 23:15:09, hanwenn wrote:
> > >
> https://codereview.appspot.com/555420043/diff/549710049/lily/text-interface.cc
> > > File lily/text-interface.cc (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/555420043/diff/549710049/lily/text-interface.c...
> > > lily/text-interface.cc:70: SCM ligature = ly_assoc_get (substr,
> > > replacement_alist, SCM_BOOL_F);
> > > this whole thing is a bit clumsy. Couldnt we do
> > > 
> > >   for k, v in replacement-alist
> > >      str = re.sub(k, v, str)
> > > 
> > > (python code, but you get the idea.)
> > 
> > While simpler, your code does not do the same. The currently implemented
> > algorithm is really text replacement per textbook: Try the longest
replacement
> > first to make sure we match the longest substring at each position. This
> > warrants the "clumsy" structure as you call it.
> 
> Well, what if you sort the replacements by decreasing size?

Yes, but you still have to check every position and not do a global
search-and-replace.

> If you want to keep this around, can you add a comment why it needs to be like
> this, 
> with an example where my simplistic approach wouldn't work.
Sign in to reply to this message.

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