C++ cleanup : get rid of some compilation warnings

C++ cleanup : get rid of some compilation warnings

This is just a minor patch destined to get rid of
most (non -Wconversion) compilation warnings.

There’s still a -Wsequence-point warning about
an undefined operation in page-turn-page-breaking.cc,
but that one’s out of reach for me.

[Valentin Villenave, 2019-02-18 23:30:24]

Indentation

[dak, 2019-02-19 12:55:11]

https://codereview.appspot.com/353880043/diff/2/lily/beam-quanting.cc
File lily/beam-quanting.cc (right):

https://codereview.appspot.com/353880043/diff/2/lily/beam-quanting.cc#newcode...
lily/beam-quanting.cc:1039: string card = best->score_card_ + to_string ("
c%d/%lu", completed, configs.size ());
I think that %lu is not guaranteed platform-independently to correspond to
size_t (or whatever we have here).  There may be some special letter for it, or
maybe it is only a GNU extension.

[Initial thought, later revised:] Maybe bite the bullet and use
string card = (std::stringstream (best->score_card_) << " c" << completed << "/"
<< configs.size ()).str ();
instead?  It's not what we use elsewhere (yet) but it should be comparatively
future-proof and is what C++ is supposed to use.

It would need #include <sstream> to work.
[Revision: use %zu, see later for rationale]

https://codereview.appspot.com/353880043/diff/2/lily/main.cc
File lily/main.cc (right):

https://codereview.appspot.com/353880043/diff/2/lily/main.cc#newcode635
lily/main.cc:635: { }; // ignore option for backwards compatibility
This code is insane.  Maybe just outcomment the whole else if branch instead?

https://codereview.appspot.com/353880043/diff/2/lily/text-interface.cc
File lily/text-interface.cc (right):

https://codereview.appspot.com/353880043/diff/2/lily/text-interface.cc#newcod...
lily/text-interface.cc:176: non_fatal_error (_f ("Markup depth exceeds maximal
value of %lu; "
Ugh.  stringstream does not really work sensibly with _f, does it?  Scratch the
previous idea.  Wouldn't this then need %zu instead?  Assuming that C++ is
following C99 in that regard?

[lemzwerg, 2019-02-19 13:46:56]

https://codereview.appspot.com/353880043/diff/2/lily/main.cc
File lily/main.cc (right):

https://codereview.appspot.com/353880043/diff/2/lily/main.cc#newcode635
lily/main.cc:635: { }; // ignore option for backwards compatibility
On 2019/02/19 12:55:10, dak wrote:
> This code is insane.  Maybe just outcomment the whole else if branch instead?

Well, option `--relocate' should (1) be recognized (i.e., it shouldn't produce
an error), but at the same time it should (2) be a no-op.  Removing the branch
would disable (1)...

[dak, 2019-02-19 13:53:54]

On 2019/02/19 13:46:56, lemzwerg wrote:
> https://codereview.appspot.com/353880043/diff/2/lily/main.cc
> File lily/main.cc (right):
> 
> https://codereview.appspot.com/353880043/diff/2/lily/main.cc#newcode635
> lily/main.cc:635: { }; // ignore option for backwards compatibility
> On 2019/02/19 12:55:10, dak wrote:
> > This code is insane.  Maybe just outcomment the whole else if branch
instead?
> 
> Well, option `--relocate' should (1) be recognized (i.e., it shouldn't produce
> an error), but at the same time it should (2) be a no-op.  Removing the branch
> would disable (1)...

Where do you see any action taken differently in reaction to "recognizing"
--relocate?  This just runs into break; either way.

[lemzwerg, 2019-02-19 14:11:57]

> Where do you see any action taken differently in reaction
> to "recognizing" --relocate?  This just runs into break;
> either way.

Ah, right.  My mistake.  On the other hand, maybe we should add a warning that
`--relocate' is a deprecated no-op?

[Valentin Villenave, 2019-02-19 21:40:03]

Use %zu; add warning for --relocate.

[Valentin Villenave, 2019-02-19 21:42:45]

On 2019/02/19 21:40:03, Valentin Villenave wrote:
> Use %zu; add warning for --relocate.

BTW, if anyone has an idea about the -Wsequence-point warning issued about
is_break () in page-turn-page-breaking.cc, I’d gladly welcome a helping hand!

V.

[dak, 2019-02-19 22:29:34]

v.villenave@gmail.com writes:

> On 2019/02/19 21:40:03, Valentin Villenave wrote:
>> Use %zu; add warning for --relocate.
>
> BTW, if anyone has an idea about the -Wsequence-point warning issued
> about is_break () in page-turn-page-breaking.cc, I’d gladly welcome a
> helping hand!

I get no warning there.

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


-- 
David Kastrup

[Valentin Villenave, 2019-02-19 23:21:17]

On 2019/02/19 22:29:34, dak wrote:
> I get no warning there.

You may need to `make clean’ or `make bin-clean’ then `make’ again to get it:


page-turn-page-breaking.cc: In instantiation of 'bool is_break(T*) [with T =
Grob]':
page-turn-page-breaking.cc:50:54:   required from here
page-turn-page-breaking.cc:38:3: warning: operation on '*0' may be undefined
[-Wsequence-point]
   if (turnable
   ^~
page-turn-page-breaking.cc: In instantiation of 'bool is_break(T*) [with T =
Prob]':
page-turn-page-breaking.cc:50:54:   required from here
page-turn-page-breaking.cc:38:3: warning: operation on '*0' may be undefined
[-Wsequence-point]


V.

[dak, 2019-02-19 23:44:40]

v.villenave@gmail.com writes:

> On 2019/02/19 22:29:34, dak wrote:
>> I get no warning there.
>
> You may need to `make clean’ or `make bin-clean’ then `make’ again to
> get it:
>
>
> page-turn-page-breaking.cc: In instantiation of 'bool is_break(T*) [with
> T = Grob]':
> page-turn-page-breaking.cc:50:54:   required from here
> page-turn-page-breaking.cc:38:3: warning: operation on '*0' may be
> undefined [-Wsequence-point]
>    if (turnable
>    ^~
> page-turn-page-breaking.cc: In instantiation of 'bool is_break(T*) [with
> T = Prob]':
> page-turn-page-breaking.cc:50:54:   required from here
> page-turn-page-breaking.cc:38:3: warning: operation on '*0' may be
> undefined [-Wsequence-point]

No idea.

-- 
David Kastrup

[John Mandereau, 2019-02-21 19:42:23]

v.villenave@gmail.com wrote:

> page-turn-page-breaking.cc: In instantiation of 'bool is_break(T*)
> [with
> T = Grob]':
> page-turn-page-breaking.cc:50:54:   required from here
> page-turn-page-breaking.cc:38:3: warning: operation on '*0' may be
> undefined [-Wsequence-point]
>     if (turnable
>     ^~
> page-turn-page-breaking.cc: In instantiation of 'bool is_break(T*)
> [with
> T = Prob]':
> page-turn-page-breaking.cc:50:54:   required from here
> page-turn-page-breaking.cc:38:3: warning: operation on '*0' may be
> undefined [-Wsequence-point]


FWIW I get this warning as well (building on Fedora 29 with GCC 8.2.1).

Best,
John

[dak, 2019-02-21 19:59:33]

John Mandereau <john.mandereau@gmail.com> writes:

> v.villenave@gmail.com wrote:
>
>> page-turn-page-breaking.cc: In instantiation of 'bool is_break(T*)
>> [with
>> T = Grob]':
>> page-turn-page-breaking.cc:50:54:   required from here
>> page-turn-page-breaking.cc:38:3: warning: operation on '*0' may be
>> undefined [-Wsequence-point]
>>     if (turnable
>>     ^~
>> page-turn-page-breaking.cc: In instantiation of 'bool is_break(T*)
>> [with
>> T = Prob]':
>> page-turn-page-breaking.cc:50:54:   required from here
>> page-turn-page-breaking.cc:38:3: warning: operation on '*0' may be
>> undefined [-Wsequence-point]
>
>
> FWIW I get this warning as well (building on Fedora 29 with GCC 8.2.1).

touch lily/page-turn-page-breaking.cc
CPU_COUNT=9 make -j9
[...]
make[1]: Leaving directory '/usr/local/tmp/lilypond/flower'
make[1]: Entering directory '/usr/local/tmp/lilypond/lily'
rm -f ./out/page-turn-page-breaking.dep;
DEPENDENCIES_OUTPUT="./out/page-turn-page-breaking.dep
./out/page-turn-page-breaking.o" g++ -c -Woverloaded-virtual 
-I/usr/include/python2.7 -I/usr/include/x86_64-linux-gnu/python2.7 
-fno-strict-aliasing -g
-fdebug-prefix-map=/build/python2.7-YmcQQ3/python2.7-2.7.15=.
-fstack-protector-strong  -g -fwrapv  -DHAVE_CONFIG_H  -DDEBUG -I./include
-I./out -I../flower/include -I../flower/./out -I../flower/include  -O2
-finline-functions -g -pipe -Wno-cast-function-type
-I/usr/local/tmp/guile-1.8/include  -pthread -I/usr/include/freetype2
-I/usr/include/libpng16 -I/usr/include/pango-1.0 -I/usr/include/harfbuzz
-I/usr/include/pango-1.0 -I/usr/include/fribidi -I/usr/include/glib-2.0
-I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/uuid
-I/usr/include/freetype2 -I/usr/include/libpng16  -W -Wall -Wconversion -o
out/page-turn-page-breaking.o page-turn-page-breaking.cc
make -C ../flower &&  true
make[2]: Entering directory '/usr/local/tmp/lilypond/flower'
true
make[2]: Leaving directory '/usr/local/tmp/lilypond/flower'
[...]
g++ --version
g++ (Ubuntu 8.2.0-21ubuntu1) 8.2.0

head config.log
This file contains any messages produced by compilers while
running configure, to aid debugging if configure makes a mistake.

It was created by configure, which was
generated by GNU Autoconf 2.69.  Invocation command line was

  $ ./configure GUILE_CONFIG=/usr/local/tmp/guile-1.8/bin/guile-config
--enable-checking --no-create --no-recursion

## --------- ##
## Platform. ##


Huh.  Maybe the Ubuntu compilation of gcc/g++ disabled some warnings?

g++ --verbose
Using built-in specs.
COLLECT_GCC=g++
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/8/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu 8.2.0-21ubuntu1'
--with-bugurl=file:///usr/share/doc/gcc-8/README.Bugs
--enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++ --prefix=/usr
--with-gcc-major-version-only --program-suffix=-8
--program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id
--libexecdir=/usr/lib --without-included-gettext --enable-threads=posix
--libdir=/usr/lib --enable-nls --enable-bootstrap --enable-clocale=gnu
--enable-libstdcxx-debug --enable-libstdcxx-time=yes
--with-default-libstdcxx-abi=new --enable-gnu-unique-object
--disable-vtable-verify --enable-libmpx --enable-plugin --enable-default-pie
--with-system-zlib --with-target-system-zlib --enable-objc-gc=auto
--enable-multiarch --disable-werror --with-arch-32=i686 --with-abi=m64
--with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic
--enable-offload-targets=nvptx-none --without-cuda-driver
--enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu
--target=x86_64-linux-gnu
Thread model: posix
gcc version 8.2.0 (Ubuntu 8.2.0-21ubuntu1) 

Huh.  No idea.

-- 
David Kastrup

[John Mandereau, 2019-02-21 23:04:30]

David Kastrup wrote:
> Huh.  Maybe the Ubuntu compilation of gcc/g++ disabled some warnings?
> 
> g++ --verbose
> Using built-in specs.
> COLLECT_GCC=g++
> COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/8/lto-wrapper
> OFFLOAD_TARGET_NAMES=nvptx-none
> OFFLOAD_TARGET_DEFAULT=1
> Target: x86_64-linux-gnu
> Configured with: ../src/configure -v --with-pkgversion='Ubuntu 8.2.0-
> 21ubuntu1' --with-bugurl=file:///usr/share/doc/gcc-8/README.Bugs --
> enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++ --
> prefix=/usr --with-gcc-major-version-only --program-suffix=-8 --
> program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-
> build-id --libexecdir=/usr/lib --without-included-gettext --enable-
> threads=posix --libdir=/usr/lib --enable-nls --enable-bootstrap --
> enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-
> time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object
> --disable-vtable-verify --enable-libmpx --enable-plugin --enable-
> default-pie --with-system-zlib --with-target-system-zlib --enable-
> objc-gc=auto --enable-multiarch --disable-werror --with-arch-32=i686
> --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --
> with-tune=generic --enable-offload-targets=nvptx-none --without-cuda-
> driver --enable-checking=release --build=x86_64-linux-gnu --
> host=x86_64-linux-gnu --target=x86_64-linux-gnu
> Thread model: posix
> gcc version 8.2.0 (Ubuntu 8.2.0-21ubuntu1) 
> 
> Huh.  No idea.

On my build host g++ --verbose says

Using built-in specs.
COLLECT_GCC=/usr/bin/g++
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/8/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-redhat-linux
Configured with: ../configure --enable-bootstrap --enable-
languages=c,c++,fortran,objc,obj-c++,ada,go,lto --prefix=/usr --
mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bu
gzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --
enable-checking=release --enable-multilib --with-system-zlib --enable-
__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object
--enable-linker-build-id --with-gcc-major-version-only --with-linker-
hash-style=gnu --enable-plugin --enable-initfini-array --with-isl --
enable-libmpx --enable-offload-targets=nvptx-none --without-cuda-driver 
--enable-gnu-indirect-function --enable-cet --with-tune=generic --with-
arch_32=i686 --build=x86_64-redhat-linux
Thread model: posix
gcc version 8.2.1 20181215 (Red Hat 8.2.1-6) (GCC) 

I did "git diff --word-diff --no-index" to compare our configurations;
at first glance I didn't see anything meaningful w.r.t that warning,
which is not surprising given my newbie level in C/C++.

I also attached the output of "gcc -dumpspecs".

Best,
John

[james.lowe_runbox.com, 2019-02-22 08:41:51]

Hello,



On Fri, 22 Feb 2019 00:04:27 +0100, John Mandereau <john.mandereau@gmail.com>
wrote:

> David Kastrup wrote:
> > Huh.  Maybe the Ubuntu compilation of gcc/g++ disabled some warnings?
> > 
> > g++ --verbose
> > Using built-in specs.
> > COLLECT_GCC=g++
> > COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/8/lto-wrapper
> > OFFLOAD_TARGET_NAMES=nvptx-none
> > OFFLOAD_TARGET_DEFAULT=1
> > Target: x86_64-linux-gnu
> > Configured with: ../src/configure -v --with-pkgversion='Ubuntu 8.2.0-
> > 21ubuntu1' --with-bugurl=file:///usr/share/doc/gcc-8/README.Bugs --
> > enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++ --
> > prefix=/usr --with-gcc-major-version-only --program-suffix=-8 --
> > program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-
> > build-id --libexecdir=/usr/lib --without-included-gettext --enable-
> > threads=posix --libdir=/usr/lib --enable-nls --enable-bootstrap --
> > enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-
> > time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object
> > --disable-vtable-verify --enable-libmpx --enable-plugin --enable-
> > default-pie --with-system-zlib --with-target-system-zlib --enable-
> > objc-gc=auto --enable-multiarch --disable-werror --with-arch-32=i686
> > --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --
> > with-tune=generic --enable-offload-targets=nvptx-none --without-cuda-
> > driver --enable-checking=release --build=x86_64-linux-gnu --
> > host=x86_64-linux-gnu --target=x86_64-linux-gnu
> > Thread model: posix
> > gcc version 8.2.0 (Ubuntu 8.2.0-21ubuntu1) 
> > 
> > Huh.  No idea.
> 
> On my build host g++ --verbose says
> 
> Using built-in specs.
> COLLECT_GCC=/usr/bin/g++
> COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/8/lto-wrapper
> OFFLOAD_TARGET_NAMES=nvptx-none
> OFFLOAD_TARGET_DEFAULT=1
> Target: x86_64-redhat-linux
> Configured with: ../configure --enable-bootstrap --enable-
> languages=c,c++,fortran,objc,obj-c++,ada,go,lto --prefix=/usr --
> mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bu
> gzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --
> enable-checking=release --enable-multilib --with-system-zlib --enable-
> __cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object
> --enable-linker-build-id --with-gcc-major-version-only --with-linker-
> hash-style=gnu --enable-plugin --enable-initfini-array --with-isl --
> enable-libmpx --enable-offload-targets=nvptx-none --without-cuda-driver 
> --enable-gnu-indirect-function --enable-cet --with-tune=generic --with-
> arch_32=i686 --build=x86_64-redhat-linux
> Thread model: posix
> gcc version 8.2.1 20181215 (Red Hat 8.2.1-6) (GCC) 
> 
> I did "git diff --word-diff --no-index" to compare our configurations;
> at first glance I didn't see anything meaningful w.r.t that warning,
> which is not surprising given my newbie level in C/C++.
> 
> I also attached the output of "gcc -dumpspecs".

**

if it helps (another POV from my vanilla installs I use to test and merge
patches)

Ubuntu 1604LTS out of the box (with latest and greatest updates):

g++ --verbose
Using built-in specs.
COLLECT_GCC=g++
COLLECT_LTO_WRAPPER=/usr/lib/gcc/i686-linux-gnu/5/lto-wrapper
Target: i686-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu
5.4.0-6ubuntu1~16.04.11' --with-bugurl=file:///usr/share/doc/gcc-5/README.Bugs
--enable-languages=c,ada,c++,java,go,d,fortran,objc,obj-c++ --prefix=/usr
--program-suffix=-5 --enable-shared --enable-linker-build-id
--libexecdir=/usr/lib --without-included-gettext --enable-threads=posix
--libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu
--enable-libstdcxx-debug --enable-libstdcxx-time=yes
--with-default-libstdcxx-abi=new --enable-gnu-unique-object
--disable-vtable-verify --enable-libmpx --enable-plugin --with-system-zlib
--disable-browser-plugin --enable-java-awt=gtk --enable-gtk-cairo
--with-java-home=/usr/lib/jvm/java-1.5.0-gcj-5-i386/jre --enable-java-home
--with-jvm-root-dir=/usr/lib/jvm/java-1.5.0-gcj-5-i386
--with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-5-i386
--with-arch-directory=i386 --with-ecj-jar=/usr/share/java/eclipse-ecj.jar
--enable-objc-gc --enable-targets=all --enable-multiarch --disable-werror
--with-arch-32=i686 --with-multilib-list=m32,m64,mx32 --enable-multilib
--with-tune=generic --enable-checking=release --build=i686-linux-gnu
--host=i686-linux-gnu --target=i686-linux-gnu
Thread model: posix
gcc version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.11) 




Ubuntu 1804LTS out of the box (with latest and greatest updates):
++ --verbose
Using built-in specs.
COLLECT_GCC=g++
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/8/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu 8.2.0-7ubuntu1'
--with-bugurl=file:///usr/share/doc/gcc-8/README.Bugs
--enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++ --prefix=/usr
--with-gcc-major-version-only --program-suffix=-8
--program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id
--libexecdir=/usr/lib --without-included-gettext --enable-threads=posix
--libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu
--enable-libstdcxx-debug --enable-libstdcxx-time=yes
--with-default-libstdcxx-abi=new --enable-gnu-unique-object
--disable-vtable-verify --enable-libmpx --enable-plugin --enable-default-pie
--with-system-zlib --with-target-system-zlib --enable-objc-gc=auto
--enable-multiarch --disable-werror --with-arch-32=i686 --with-abi=m64
--with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic
--enable-offload-targets=nvptx-none --without-cuda-driver
--enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu
--target=x86_64-linux-gnu
Thread model: posix
gcc version 8.2.0 (Ubuntu 8.2.0-7ubuntu1) 

***

James

[Valentin Villenave, 2019-02-25 16:39:55]

Pushed as
https://git.savannah.gnu.org/cgit/lilypond.git/commit/?id=e94db23e6c620396aaf...
Thanks everyone!

V.