[espeak-ng:master] reported: Issue 945 #github


espeak-ng@groups.io Integration <espeak-ng@...>
 

[espeak-ng:master] New Comment on Pull Request #955 Issue 945
By rhdunn:

I've noticed -- especially in 2d3cc580b8f837aabad369665f677846490ccdbd -- that there are a lot of extraneous whitespace changes, changing spaces to tabs in comments. The general rule in the espeak-ng codebase is "tabs to indent, spaces to align" so tabs should only appear at the start of lines and not in comments.


espeak-ng@groups.io Integration <espeak-ng@...>
 

[espeak-ng:master] New Comment on Pull Request #955 Issue 945
By jbowler:

[@rdunn] "It is easier to understand a commit if it is making one change instead of several. Specifically, this commit could have been several separate commits"

There are 10 separate commits there. The main ones are required in sequence, but things like the first to allow "make check" to succeed can be skipped by cherry picking (git cherry-pick). It's just that I don't have a compiler which succeeds with that bug present so, since it is known and unrelated, I had to temporarily remove the check.


espeak-ng@groups.io Integration <espeak-ng@...>
 

[espeak-ng:master] New Comment on Pull Request #955 Issue 945
By rhdunn:

I'm having a hard time understanding the last commit, as it is making a lot of changes and I cannot wrap my head around what has changed from looking at the diff only. Having that broken up into separate commits would make it more manageable to review. Also, each commit is doing multiple things, which makes it harder for someone looking at the code (especially if a git bisect identifies one of these commits as breaking something) to verify that it is correct and if not what is wrong with it.


espeak-ng@groups.io Integration <espeak-ng@...>
 

[espeak-ng:master] New Comment on Pull Request #955 Issue 945
By rhdunn:

I've pushed fixes for the various warnings, including the -Wint-conversion warning. -- That's the kind of thing I'm looking for, where each commit does one thing (constify variables, isolate phoneme list table/list sizes, fix a type of warning, etc.).

Note: You can use make CFLAGS="-Wall" to pass additional flags to the build outside of the autoconf/automake process. For example:

make CFLAGS="-Werror -Wall -Wextra -Wno-format-overflow -Wno-format-truncation -Wno-format -Wno-sign-compare -Wno-implicit-fallthrough"

It is generally cleaner commit-wise to rebase and edit the commits. It may also be easier (especially for reviewers) to split this into smaller PRs, possibly around the different commits you have.