Fix type mismatch warnings #839
Conversation
Many files produced many type mismatch warnings with both clang 17 and gcc 14 on a Debian 6.10.11 x86_64 system. This commit fixes almost all the ones I'm seeing in the TFQ code base. Some other warnings still arise in other code and modules. Those warnings don't appear to be things we can easily work around short of creating a lot of patch files (at significant cost in time and testing). For those, I'm currently relying on setting compiler options (e.g., `-Wno-unused-function` for warnings about unused functions) narrowed in scope to the problematic package using Bazel's `--per_file_copt` and `--host_per_file_copt` flags. Those flags are not in this commit; they need to be made part of the TFQ `configure.sh` script.
mhucka
added
kind/chore
MichaelBroughton
left a comment
MichaelBroughton
left a comment
There was a problem hiding this comment.
Looks good. Hopefully this gets rid of some of the type warning headaches. It does seem like our use of size_t vs int in loops now will be more inconsistent overall, but if that gets us fewer warnings, I guess it's a step in the right direction.
| for (int i = start; i < end; i++) { | ||
| for (int j = 0; j < n_symbols; j++) { | ||
| for (int k = 0; k < output_results.at(i).at(j).size(); k++) { | ||
| for (size_t k = 0; k < output_results.at(i).at(j).size(); k++) { |
There was a problem hiding this comment.
Why do just k and not i, j as well ?
There was a problem hiding this comment.
Ah, that's because k is the only one being compared to a .size() value; i and j are used as indexes.
| for (size_t k = 0; k < output_results.at(i).at(j).size(); k++) { | ||
| output_tensor(i, j, k) = output_results.at(i).at(j).at(k); | ||
| } | ||
| for (int k = output_results.at(i).at(j).size(); |
There was a problem hiding this comment.
I agree this one looks inconsistent. It's because in this case, the value of k is being compared to largest_single_symbol on the next line, and that variable is an int.
The weird thing is k is initialized by setting its value to the output of size(), and yet the compiler isn't complaining about that part. I don't have a good explanation for that.
Although the compiler didn't flag them, a couple of declarations that were changed to size_t in a previous commit should probably have const declarations, to be consistent with other changes in the same function.
One of the `size_t` → `int` changes was wrong and led to a test failure. I would prefer to edit the commit itself; however, that would require force-pushing to the TFQ repo, and I'm not sure if that will break the code review comments in the PR in GitHub. So I'm making the change in a separate commit.
2 participants
Many files produced many type mismatch warnings with both clang 17 and gcc 14 on a Debian 6.10.11 x86_64 system. This commit fixes almost all the ones I'm seeing in the TFQ code base.
Some other warnings still arise in other code and modules. Those warnings don't appear to be things we can easily work around short of creating a lot of patch files (at significant cost in time and testing). For those, I'm currently relying on setting compiler options (e.g.,
-Wno-unused-functionfor warnings about unused functions) narrowed in scope to the problematic package using Bazel's--per_file_coptand--host_per_file_coptflags. Those flags are not in this PR.Note: this PR should be applied after PR #837.