Prefer size_t over int where possible#415
Conversation
AZero13
force-pushed
the
work
branch
5 times, most recently
from
cef6f59 to
cca060e
Compare
|
These fixes look mostly good. Though, some cases the refactoring changes the structure ... Comments inline to for @paulusmack to review. |
|
I have mixed feelings about these changes. It's reasonable to use size_t for things that are the length of a structure or a string, but I don't think that the savings in code size will be significant either in space or time. There's also a lot of changes spread out across the source tree which could be better as separate commits. |
I think they would be best done at once, that's why, since they all have the same theme. |
|
I notice also that the signoff is a noreply.github.com address, which I don't consider adequate. |
AZero13
force-pushed
the
work
branch
2 times, most recently
from
dc9e33e to
d1cac16
Compare
|
@Neustradamus Fixed! |
paulusmack
left a comment
paulusmack
left a comment
There was a problem hiding this comment.
In general it seems like there is a bunch of refactoring here plus gratuitous whitespace changes, in addition to the changes mentioned in the commit description (i.e. converting int to size_t). So, at the very least the commit description needs to be more comprehensive. If nothing else it should identify any potential bugs fixed here, though in fact I would prefer the bug fixes to be in separate commits.
I'm not convinced by the rationale that we need to do this to reduce the generated code size. As far as I can see it is only going to reduce the code size on 64-bit machines, not 32-bit machines, some of which might arguably be a bit short on memory. If you have a 64-bit processor you probably have a GB or more of memory, and saving at most a few hundred bytes of instruction memory is going to be in the noise.
|
@AtariDreams: Have you seen @paulusmack comments started: Note: The other PR has been merged :) |
|
@AtariDreams: Have you looked comments from @paulusmack? |
|
I'll take a look. |
|
As far as I can see, there is no functional change here (or at least, none intended), so I am not going to wait for this. |
This avoids needless truncation, which takes up multiple more instructions on some architectures, especially older ones. Signed-off-by: Rose <gfunni234@gmail.com>
|
@paulusmack: Have you seen the @RSilicon changes? |
|
|
||
| for (uep = userenv_list; uep != NULL; uep = uep->ue_next) { | ||
| int i; | ||
| size_t i; |
There was a problem hiding this comment.
For a case like this where the variable is a loop index rather than the size or length of something, I'd prefer plain long or unsigned long rather than size_t. Same comment applies in a few other places.
| } | ||
| va_end(ap); | ||
| if (best > -1) (*pos) += best_len; | ||
| (*pos) += best_len; |
There was a problem hiding this comment.
This is a semantic change; previously the function would return -1 if the string at *pos didn't match any of the argument strings at all, whereas now it will return 0, which callers interpret as a match with the second argument. So now text2qos will parse things incorrectly.
This is an example of why I would have preferred this commit to be split into smaller pieces, for example, four separate commits for chat, pppd, and the pppoatm and pppoe plugins. If you had, I could merge the commits that are OK. As it is, I can't merge any of it.
|
Overall, I would take everything except the pppoatm changes, which don't seem to me to be at all necessary, and in fact wrong, as I commented. |
|
@AZero13: Have you seen the latest @paulusmack comments? |
|
Yes I will address this now |
|
@AZero13: Have you progressed on it? |
|
@AZero13: It is the end of August 2025, have you progressed on it? |
|
@paulusmack: With the silence of this PR author, maybe you can look to adapt this PR directly? |
|
@AZero13: I have sent you e-mails about your PR. Can you look your PR? Thanks in advance. |
|
Okay, okay okay I will. Yes I will do this |
|
@LumioseSil: Can you check it? Paul prepares a new build... |
5 participants
This avoids needless truncation, which takes up multiple more instructions on some architectures, especially older ones.