Use size_t for Process offset values #1588
Conversation
Explorer09
force-pushed
the
process-offsets-size-type
branch
from
454cf1e to
87ab00e
Compare
Process.c
Outdated
| @@ -1057,7 +1057,7 @@ void Process_updateExe(Process* this, const char* exe) { | |||
| if (exe) { | |||
| this->procExe = xStrdup(exe); | |||
| const char* lastSlash = strrchr(exe, '/'); | |||
| this->procExeBasenameOffset = (lastSlash && *(lastSlash + 1) != '\0' && lastSlash != exe) ? (size_t)(lastSlash - exe + 1) : 0; | |||
| this->procExeBasenameOffset = (lastSlash > exe && *(lastSlash + 1) != '\0') ? (size_t)(lastSlash - exe + 1) : 0; | |||
There was a problem hiding this comment.
Side note: I have no idea why this conditional needs a lastSlash != exe. I thought it should be legitimate when lastSlash == exe, that is, when the exe string begins with a slash such as "/vmlinuz".
BenBE
added
enhancement
|
Well, it looks like the changes to The I think the logic there needs to be revised. While I could workaround by using
EDIT: This change has been done. |
Explorer09
force-pushed
the
process-offsets-size-type
branch
2 times, most recently
from
c0c9a0b to
707cde1
Compare
|
FYI; the last time I needed to work on this logic, it took me the better of 3 months to track down some issue with the command line handling in this code. So be careful with your changes; the details are important with this part of the code … |
|
@BenBE But I'm not sure I should file a new PR for it or just merge with this one. The new changes depend on this PR and I've seen you added tags and categories to this PR already. |
|
@Explorer09 Make a second PR as this makes discussing the changes easier. From a quick view at your commits on that branch I already noticed some things I'd like to change with that patchset … |
Explorer09
force-pushed
the
process-offsets-size-type
branch
from
707cde1 to
473c847
Compare
Explorer09
force-pushed
the
process-offsets-size-type
branch
2 times, most recently
from
8b874d5 to
c256d1e
Compare
There was a problem hiding this comment.
Will take me some time to check consistency with all those edits. Especially with all the changes regarding edge cases re signed-nes changing due to this PR.
What might be worth doing is having the markers for "not used" become a special constant #define STRPOS_UNUSED ((size_t)-1).
| if (cmdlineBasenameStart < 0) | ||
| return false; |
There was a problem hiding this comment.
This removes a check when no basename was found in the command line …
There was a problem hiding this comment.
"No basename found" is no different from matching "comm" from the start of the "cmdline" string. You notice this function does not take cmdlineBasenameLen as input. "No basename found" may be indicated by cmdlineBasenameLen == 0, but it's irrelevant to this function.
There was a problem hiding this comment.
Just checking the change in the code has been accounted for in a different way.
| if (delim == 0 || delim == '\n' || delim == ' ') { | ||
| int i, j; | ||
| size_t i, j; | ||
| /* reverse match the cmdline prefix and exe suffix */ | ||
| for (i = cmdlineBaseOffset - 1, j = exeBaseOffset - 1; |
There was a problem hiding this comment.
This could underflow for both i and j …
There was a problem hiding this comment.
Intentional. i and j may wrap over to (size_t)-1 and that's what the conditionals below are for.
There was a problem hiding this comment.
Don't quite like doing it that way …
Especially as x >= 0 is a no-op (always true) for any unsigned x …
There was a problem hiding this comment.
Do you have any better way here? Note that the i >= 0 had been rewritten to i != (size_t)-1 by me when I changed the types to unsigned.
Reorder a conditional to prevent an unnecessary negative array offset access. The compiled behavior should be exactly the same. Signed-off-by: Kang-Che Sung <[email protected]>
No code changes. Fix an error by commit 94a52cb that accidentally replaced a keyword in comment to "cmdlineBasenameEnd". According to the context it should refer to "cmdlineBasenameStart" instead.
Improve readability of local variables. Signed-off-by: Kang-Che Sung <[email protected]>
Store the "cmdlineBasenameLen" and "commLen" local variables rather than "cmdlineBasenameEnd" and "commEnd". Signed-off-by: Kang-Che Sung <[email protected]>
Adjust logic in Process_makeCommandStr() so that if the process "cmdline" basename has a length of zero, skip creating all of the highlights of that basename. Signed-off-by: Kang-Che Sung <[email protected]>
The matchCmdlinePrefixWithExeSuffix() internal function (in Process.c) can match basenames in multiple tries in case the "cmdlineBasenameStart" input value is unreliable. Take advantage of this and update "cmdlineBasenameStart" with the new offset detected by the function. Also make the matching behavior consistent regardless of "showMergedCommand" setting. Signed-off-by: Kang-Che Sung <[email protected]>
Add a special case when the process "comm" string is found in "cmdline", but at the starting basename position that would be stripped by "stripExeFromCmdline" setting. This special case will now highlight the basename of "exe" string as well as the first token of "cmdline" (as the two strings are concatenated together when displaying). Signed-off-by: Kang-Che Sung <[email protected]>
Convert the members "cmdlineBasenameStart", "cmdlineBasenameEnd" and "procExeBasenameOffset" in "Process" to size_t type. Also upgrade many routines that search for "basenames", COMM, and PROC_EXE string to use size_t for iterators. The "cmdlineBasenameEnd" value is no longer allowed to negative. It is now set to 0 during Process_init(). Signed-off-by: Kang-Che Sung <[email protected]>
Explorer09
force-pushed
the
process-offsets-size-type
branch
from
c256d1e to
cc24137
Compare
Convert
cmdlineBasenameStart,cmdlineBasenameEndandprocExeBasenameOffsetvalues inProcesstosize_ttype. Improve functions that search for "basenames", "comm" and "procExe" to usesize_tfor offsets as well.I made these set of code changes before the length properties of
RichStringobject can be upgraded tosize_t.Note: This PR contains many platform specific code changes. I didn't test all of them. It is possible that I broke something.