Make Pitch::to_string() more robust

Make Pitch::to_string() more robust

[lemzwerg, 2020-02-01 07:01:21]

LGTM

[Dan Eble, 2020-02-01 11:19:12]

https://codereview.appspot.com/581580043/diff/557260048/lily/pitch.cc
File lily/pitch.cc (right):

https://codereview.appspot.com/581580043/diff/557260048/lily/pitch.cc#newcode162
lily/pitch.cc:162: if (qt >= 0 && qt < int (sizeof(accname) / sizeof(const
char*))) {
Instead of casting to int and checking that the value is non-negative, why not 

  size_t qt = ...
  if (qt < ...)

A pre-C++11 way to improve computing the array size is not to repeat the element
type:

  sizeof (accname) / sizeof (accname[0])

A C++11 way to simplify the code that uses the size is to use std::array, though
it would come at the cost of declaring the count.

  #include <array>
  ...
  static const std::array<char const *, 9> = {"eses" ...
  ...
  size_t qt = ...
  if (qt < accname.size ())

https://codereview.appspot.com/581580043/diff/557260048/lily/pitch.cc#newcode162
lily/pitch.cc:162: if (qt >= 0 && qt < int (sizeof(accname) / sizeof(const
char*))) {
Space after sizeof?

[hanwenn, 2020-02-01 21:07:31]

Dan's comments

[hanwenn, 2020-02-01 21:09:18]

https://codereview.appspot.com/581580043/diff/557260048/lily/pitch.cc
File lily/pitch.cc (right):

https://codereview.appspot.com/581580043/diff/557260048/lily/pitch.cc#newcode162
lily/pitch.cc:162: if (qt >= 0 && qt < int (sizeof(accname) / sizeof(const
char*))) {
On 2020/02/01 11:19:12, Dan Eble wrote:
> Space after sizeof?

clang-format ftw!

done.

https://codereview.appspot.com/581580043/diff/557260048/lily/pitch.cc#newcode162
lily/pitch.cc:162: if (qt >= 0 && qt < int (sizeof(accname) / sizeof(const
char*))) {
On 2020/02/01 11:19:12, Dan Eble wrote:
> Instead of casting to int and checking that the value is non-negative, why not

> 
>   size_t qt = ...
>   if (qt < ...)
> 
> A pre-C++11 way to improve computing the array size is not to repeat the
element
> type:
> 
>   sizeof (accname) / sizeof (accname[0])
> 

thanks, done.

> A C++11 way to simplify the code that uses the size is to use std::array,
though
> it would come at the cost of declaring the count.

Decline; then I might as well just write 

  if (qt < 9) ..

[Dan Eble, 2020-02-01 21:19:46]

I approve of your use of braces for even single-statement blocks.

https://codereview.appspot.com/581580043/diff/561400052/lily/pitch.cc
File lily/pitch.cc (right):

https://codereview.appspot.com/581580043/diff/561400052/lily/pitch.cc#newcode162
lily/pitch.cc:162: if (qt < int (sizeof (accname) / sizeof (accname[0])))
seems like you shouldn't need to cast it to int anymore
aside from that, LGTM

[hanwenn, 2020-02-01 21:26:13]

dan, round 2

[hanwenn, 2020-02-01 21:26:36]

https://codereview.appspot.com/581580043/diff/561400052/lily/pitch.cc
File lily/pitch.cc (right):

https://codereview.appspot.com/581580043/diff/561400052/lily/pitch.cc#newcode162
lily/pitch.cc:162: if (qt < int (sizeof (accname) / sizeof (accname[0])))
On 2020/02/01 21:19:46, Dan Eble wrote:
> seems like you shouldn't need to cast it to int anymore
> aside from that, LGTM

Done.