Re: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ballads@gmail.com)

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ballads@gmail.com)

Dev mailing list
Reply | Threaded
Open this post in threaded view
|

Re: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ballads@gmail.com)

jonas.hahnfeld
Fabulous, thanks for putting this together!

https://codereview.appspot.com/553310045/

dak
Reply | Threaded
Open this post in threaded view
|

Re: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ballads@gmail.com)

dak
In reply to this post by Dev mailing list
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/

Reply | Threaded
Open this post in threaded view
|

Re: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ballads@gmail.com)

Hans Åberg-2
In reply to this post by Dev mailing list

> On 14 Dec 2019, at 09:54, lemzwerg--- via Discussions on LilyPond development <[hidden email]> 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);
    …
  }



Reply | Threaded
Open this post in threaded view
|

Re: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ballads@gmail.com)

Hans Åberg-2
In reply to this post by dak

> On 14 Dec 2019, at 13:07, [hidden email] 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.



Reply | Threaded
Open this post in threaded view
|

RE: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ballads@gmail.com)

lilypond-5
In reply to this post by Dev mailing list
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


Reply | Threaded
Open this post in threaded view
|

Re: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ballads@gmail.com)

nine.fierce.ballads
In reply to this post by Dev mailing list
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/

Reply | Threaded
Open this post in threaded view
|

RE: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ballads@gmail.com)

lilypond-5
> -----Original Message-----
> From: [hidden email] <[hidden email]>
> Sent: Saturday, December 14, 2019 8:52 PM
> To: [hidden email]; [hidden email]; [hidden email];
> [hidden email]; [hidden email]
> Cc: [hidden email]; [hidden email]
> Subject: Re: Issue 5639: compile with -std=c++11 (issue 553310045 by
> [hidden email])
>
> 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



Reply | Threaded
Open this post in threaded view
|

Re: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ballads@gmail.com)

Dan Eble
On Dec 15, 2019, at 06:31, [hidden email] 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


dak
Reply | Threaded
Open this post in threaded view
|

Re: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ballads@gmail.com)

dak
In reply to this post by Dev mailing list
On 2019/12/15 14:40:08, dan_faithful.be wrote:
> On Dec 15, 2019, at 06:31, mailto:[hidden email] 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/
Reply | Threaded
Open this post in threaded view
|

RE: Issue 5639: compile with -std=c++11 (issue 553310045 by nine.fierce.ballads@gmail.com)

lilypond-5
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: [hidden email] <[hidden email]>
> Verzonden: Monday, December 16, 2019 1:24 AM
> Aan: [hidden email]; [hidden email];
> [hidden email]; [hidden email]; [hidden email];
> [hidden email]
> CC: [hidden email]; [hidden email]
> Onderwerp: Re: Issue 5639: compile with -std=c++11 (issue 553310045 by
> [hidden email])
>
> On 2019/12/15 14:40:08, dan_faithful.be wrote:
> > On Dec 15, 2019, at 06:31, mailto:[hidden email] 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/