Prefer size_t over int where possible #415
Conversation
AreaZR
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. |
|
@AtariDreams: What is the solution? |
|
I notice also that the signoff is a noreply.github.com address, which I don't consider adequate. |
AreaZR
force-pushed
the
work
branch
2 times, most recently
from
dc9e33e
to
d1cac16
Compare
|
@Neustradamus Fixed! |
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 <[email protected]>
|
@paulusmack: Have you seen the @RSilicon changes? |
| @@ -1863,9 +1863,9 @@ update_script_environment(void) | |||
| struct userenv *uep; | |||
|
|
|||
| 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.
| @@ -37,7 +37,7 @@ int __atmlib_fetch(const char **pos,...) | |||
| } | |||
| } | |||
| 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. |
This avoids needless truncation, which takes up multiple more instructions on some architectures, especially older ones.