Fix signed-unsigned warnings#34791
Conversation
💊 CircleCI build failures summary and remediationsAs of commit 8c0d9b0 (more details on the Dr. CI page): ✅ None of the build failures appear to be your fault 💚
🚧 1 upstream failure:These were probably caused by upstream breakages:
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker. This comment has been revised 6 times. |
|
@zdevito can you take a look at this? I fear that manually fixing these warnings is a losing battle unless we come up with some broader strategy about what to do in these cases. In the past, we've forced people to use (The patch looks generally reasonable, and we don't have to block the PR itself on fixing htese things.) |
|
@ezyang would it be easier if I split this PR into few smaller ones? |
zdevito
left a comment
There was a problem hiding this comment.
I think everything looks fine here except that we should be replacing int with uint32_t and not size_t. The point is to stop the warnings, I don't want to also change indexing math to 64-bits in a way that might slow something down.
|
re warnings (not specific to this PR): the best bet is to actually get warnings clean by turning off the warnings our code is not clean against, and then gradually expand the set. Currently we just introduce warnings because they are impossible to see. |
|
@zdevito IIUC, the 32-bit/64-bit difference isn't a big deal in CPU code; it's just in CUDA where it really slays you? |
And few typos Test Plan: CI
f560bd3 to
8c0d9b0
Compare
|
Updated the diff to use |
|
@ezyang, I don't think x86_64 is capable of non-vectorized 32-bit arithmetic. |
| // Verify inputs == outputs | ||
| CAFFE_ENFORCE_EQ(init_.inputs.size(), init_.outputs.size()); | ||
| for (auto i = 0; i < init_.inputs.size(); i++) { | ||
| for (auto i = 0U; i < init_.inputs.size(); i++) { |
There was a problem hiding this comment.
Personally I prefer for (size_t i = 0; ...), but this is fine.
There was a problem hiding this comment.
yeah I think size_t is better, I'll change it after my stack of diffs land
There was a problem hiding this comment.
I mean I'll change this in my code
There was a problem hiding this comment.
Please loop @zdevito on that one, because I had size_t there initially and he suggested to use unsigned int out of performance considerations.
There was a problem hiding this comment.
But personally I agree, that if upper limit it 64-bit unsigned value, then loop iterator should also be 64-bit unsigned value.
There was a problem hiding this comment.
Actually all of params.inputs, devices_ or elements are std::vectors. While in std::vector the return type of std::vector::size() is std::vector::size_type, which in most cases is size_t. If there is platform concerns, I think we'd better use std::vector::size_type i = 0; And I don't think there should be performance issues for such a for-loop iterative variable especially the vectors should be considerable small.
And for the case in the computation code, do we have any benchmark for the differences between the type in for-loops?
facebook-github-bot
left a comment
There was a problem hiding this comment.
@malfet is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: And few typos Pull Request resolved: pytorch#34791 Test Plan: CI Differential Revision: D20524879 Pulled By: malfet fbshipit-source-id: 58fa03bd6356979e77cd1bffb6370d41a177c409
And few typos
Test Plan: CI