Tiny fixes to make gcc pedantic build happy#8933
Conversation
digantdesai
requested review from
GregoryComer,
JacobSzwejbka,
dbort,
kirklandsign,
lucylq and
manuelcandales
as code owners
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/8933
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 4c0c697 with merge base c890809 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
|
Also please update the title/commit message a bit. It has more than Fixup CMake summary message |
digantdesai
force-pushed
the
fix-warnings
branch
from
7a24b5e to
9e79452
Compare
digantdesai
changed the title
swolchok
left a comment
swolchok
left a comment
There was a problem hiding this comment.
changes requested for strncmp bug
| ET_SWITCH_SCALAR_OBJ_TYPES( | ||
| value_type, ctx, "constant_pad_nd.out", CTYPE_VALUE, [&]() { | ||
| CTYPE_VALUE val; | ||
| CTYPE_VALUE val{}; |
There was a problem hiding this comment.
what is the error message motivating this? it seems silly and reminiscent of misguided linter advice I've seen previously, but perhaps I don't know something. Is it -Wuninitialized? If so, why empty braces instead of = 0;?
There was a problem hiding this comment.
yeah it was that flag. COMPLEX_TYPES is what I was thinking about when not using =0. But I guess I can use it when there is no complex.
| for (size_t i = 0; i < delegates->size(); i++) { | ||
| auto delegate = delegates->Get(i); | ||
| if (std::strcmp(delegate->id()->c_str(), backend_name) == 0) { | ||
| if (strncmp(delegate->id()->c_str(), backend_name, delegate->id()->size()) == 0) { |
There was a problem hiding this comment.
- nit: why drop std::?
- this is incorrect -- it passes if the delegate ID is a prefix of the backend_name
There was a problem hiding this comment.
std:: is an oversight.
Prefix is a real issue, let me also check the length of the incoming in addition to strncmp.
digantdesai
force-pushed
the
fix-warnings
branch
from
9e79452 to
5110f7b
Compare
digantdesai
force-pushed
the
fix-warnings
branch
from
5110f7b to
3386f13
Compare
| auto backend_name_len = std::strlen(backend_name); | ||
| auto delegate_id_len = delegate->id()->size(); | ||
| if (backend_name_len == delegate_id_len && | ||
| std::strncmp(delegate->id()->c_str(), backend_name, backend_name_len) == | ||
| 0) { |
There was a problem hiding this comment.
you know what, I'm really confused about what error we're fixing here. how is this better than strcmp?
swolchok
left a comment
swolchok
left a comment
There was a problem hiding this comment.
unblocking, but I'd like to hear about why we can't simply use strcmp
Thanks. Basically |
IIRC this one being buggy in some random gcc version has come up previously, possibly on @mergennachin's size_test warning stuff? In general I don't think we should be held responsible for every warning in every quasi-reasonable version of every compiler because of stuff like this. |
|
yeah this was arm's bare metal gcc 13, I generally agree with you. That said since this was the only one I got tempted 😅 |
4 participants
Summary
Tiny fixes to make gcc 13 baremetal, pedantic build happy.
Test plan
CI