RE: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ball...@gmail.com)
Dan, I would certainly not want that such a typo is not corrected at all. You may take the decision, whether you push it to staging just alone (which I should prefer) or you keep it in the patchset attached to Issue 5639 > -Oorspronkelijk bericht- > Van: d...@gnu.org > Verzonden: Monday, December 16, 2019 1:24 AM > Aan: nine.fierce.ball...@gmail.com; lemzw...@googlemail.com; > jonas.hahnf...@gmail.com; haber...@telia.com; lilyp...@de-wolff.org; > d...@faithful.be > CC: lilypond-devel@gnu.org; re...@codereview-hr.appspotmail.com > Onderwerp: Re: Issue 5639: compile with -std=c++11 (issue 553310045 by > nine.fierce.ball...@gmail.com) > > On 2019/12/15 14:40:08, dan_faithful.be wrote: > > On Dec 15, 2019, at 06:31, mailto:lilyp...@de-wolff.org wrote: > > > It is not the commit title, but I do think that this is not a part > of issue > > 5639: compile with --std=c11 > > > The reason that I think it is important to keep this separated is > that the > > impact is very different. > > > When a commit with only comments is in a separate issue, it is easy > to cherry > > pick it for let say version 2.0. > > > Although you make it a separate commit, in rietveld it is still one > issue. > > > The effort of handling a separate ticket and review is not worth it to > me for > > this particular typo correction. I'm willing to revert it and leave > it for the > > next person who notices it, if that bothers you less than piggybacking > on this > > issue. > > — > > Dan > > > Dan, a typo in a comment or in a doc string _not_ passed through Texinfo or > not containing Texinfo-relevant changes is material for just pushing to > staging > in a commit of its own. As you say, review is overkill, and wrapping it into > some other topic not touching a file is a distraction. > > https://codereview.appspot.com/553310045/
Re: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ball...@gmail.com)
On 2019/12/15 14:40:08, dan_faithful.be wrote: On Dec 15, 2019, at 06:31, mailto:lilyp...@de-wolff.org wrote: > It is not the commit title, but I do think that this is not a part of issue 5639: compile with --std=c11 > The reason that I think it is important to keep this separated is that the impact is very different. > When a commit with only comments is in a separate issue, it is easy to cherry pick it for let say version 2.0. > Although you make it a separate commit, in rietveld it is still one issue. The effort of handling a separate ticket and review is not worth it to me for this particular typo correction. I'm willing to revert it and leave it for the next person who notices it, if that bothers you less than piggybacking on this issue. — Dan Dan, a typo in a comment or in a doc string _not_ passed through Texinfo or not containing Texinfo-relevant changes is material for just pushing to staging in a commit of its own. As you say, review is overkill, and wrapping it into some other topic not touching a file is a distraction. https://codereview.appspot.com/553310045/
Re: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ball...@gmail.com)
On Dec 15, 2019, at 06:31, lilyp...@de-wolff.org wrote: > It is not the commit title, but I do think that this is not a part of issue > 5639: compile with --std=c11 > The reason that I think it is important to keep this separated is that the > impact is very different. > When a commit with only comments is in a separate issue, it is easy to cherry > pick it for let say version 2.0. > Although you make it a separate commit, in rietveld it is still one issue. The effort of handling a separate ticket and review is not worth it to me for this particular typo correction. I'm willing to revert it and leave it for the next person who notices it, if that bothers you less than piggybacking on this issue. — Dan
RE: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ball...@gmail.com)
> -Original Message- > From: nine.fierce.ball...@gmail.com > Sent: Saturday, December 14, 2019 8:52 PM > To: lemzw...@googlemail.com; jonas.hahnf...@gmail.com; d...@gnu.org; > haber...@telia.com; lilyp...@de-wolff.org > Cc: lilypond-devel@gnu.org; re...@codereview-hr.appspotmail.com > Subject: Re: Issue 5639: compile with -std=c++11 (issue 553310045 by > nine.fierce.ball...@gmail.com) > > On 2019/12/14 18:23:08, lilypond_de-wolff.org wrote: > > Great job, one remark: > > Although the patch for ly/music-functions-init.ly is a good patch, I > do not > > think it should be part of this patch-set. > > > Jaap > > > It's part of the commit titled "comments." What do you suggest I do instead? > > > https://codereview.appspot.com/553310045/ [>] It is not the commit title, but I do think that this is not a part of issue 5639: compile with --std=c11 The reason that I think it is important to keep this separated is that the impact is very different. When a commit with only comments is in a separate issue, it is easy to cherry pick it for let say version 2.0. Although you make it a separate commit, in rietveld it is still one issue. Jaap
Re: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ball...@gmail.com)
> On 14 Dec 2019, at 09:54, lemzwerg--- via Discussions on LilyPond development
> wrote:
>
> LGTM, thanks!
>
> https://codereview.appspot.com/553310045/
>
The C++11 range for statement is nice too, if the iterator self is not
addressed. For example, in beam.cc:
for (auto& s: stems) {
Interval positions = Stem::head_positions(s);
…
}
Re: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ball...@gmail.com)
On 2019/12/14 18:23:08, lilypond_de-wolff.org wrote: Great job, one remark: Although the patch for ly/music-functions-init.ly is a good patch, I do not think it should be part of this patch-set. Jaap It's part of the commit titled "comments." What do you suggest I do instead? https://codereview.appspot.com/553310045/
Re: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ball...@gmail.com)
> On 14 Dec 2019, at 13:07, d...@gnu.org wrote: > > I don't want to get your enthusiasm up prematurely, but I think that the > GUB GCC versions we need(?)/use for PowerPC might not be entirely great > with C++11 though they'll likely support it when given explicitly on the > command line. From what I recall, GCC never supported C++11 as default, but skipped directly to C++14, which is the current default. The implementation of C++11 was long and troublesome, and would be better to avoid if possible on earlier compilers.
RE: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ball...@gmail.com)
Great job, one remark: Although the patch for ly/music-functions-init.ly is a good patch, I do not think it should be part of this patch-set. Jaap
Re: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ball...@gmail.com)
On 2019/12/14 10:18:33, hahnjo wrote: Fabulous, thanks for putting this together! I don't want to get your enthusiasm up prematurely, but I think that the GUB GCC versions we need(?)/use for PowerPC might not be entirely great with C++11 though they'll likely support it when given explicitly on the command line. So extensive changes may require several rebases/merges/rewrites before we are ready to put them into mainline (we don't have a separate GUB for post-2.19 work). Or we just say "to heck with it" and just assume that we don't run across bad C++11 support. That being said, one obvious contender for C++11 might be the std::vector class which we still double currently, and I think part of the reason may be that the data() member function is not guaranteed to be present before C++11. Because of the merge/rebase thing, I don't know how much of a good idea it would be to tackle this now, but it's certainly something worth doing once we are all-in on C++11. https://codereview.appspot.com/553310045/
Re: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ball...@gmail.com)
Fabulous, thanks for putting this together! https://codereview.appspot.com/553310045/
Re: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ball...@gmail.com)
LGTM, thanks! https://codereview.appspot.com/553310045/
