Found by: scan-build You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2962
-- Commit Summary --
* <a href="https://github.com/geany/geany/pull/2962/commits/b6382bd00a8fb7762d1cbfb51a0d1c36c39eebc6">Remove redundant code</a>
-- File Changes --
M src/build.c (2)
-- Patch Links --
https://github.com/geany/geany/pull/2962.patch https://github.com/geany/geany/pull/2962.diff
@xiota commented on this pull request.
@@ -1057,7 +1057,7 @@ gboolean build_parse_make_dir(const gchar *string, gchar **prefix)
if (string == NULL) return FALSE;
- if ((pos = strstr(string, "Entering directory")) != NULL) + if (strstr(string, "Entering directory") != NULL)
What about this, to avoid rescanning the beginning of the string: ```C if ((pos = strstr(string, "Entering directory")) != NULL) { // ...
pos = strstr(pos, "/"); // instead of ... pos = strstr(string, "/");
// ... } ```
@andy5995 commented on this pull request.
@@ -1057,7 +1057,7 @@ gboolean build_parse_make_dir(const gchar *string, gchar **prefix)
if (string == NULL) return FALSE;
- if ((pos = strstr(string, "Entering directory")) != NULL) + if (strstr(string, "Entering directory") != NULL)
And is there any reason not to use strchr instead of strstr?
@xiota commented on this pull request.
@@ -1057,7 +1057,7 @@ gboolean build_parse_make_dir(const gchar *string, gchar **prefix)
if (string == NULL) return FALSE;
- if ((pos = strstr(string, "Entering directory")) != NULL) + if (strstr(string, "Entering directory") != NULL)
@andy5995 Are there any tangible benefits besides maybe saving a few processor cycles or avoiding some nested function call?
@rootkea commented on this pull request.
@@ -1057,7 +1057,7 @@ gboolean build_parse_make_dir(const gchar *string, gchar **prefix)
if (string == NULL) return FALSE;
- if ((pos = strstr(string, "Entering directory")) != NULL) + if (strstr(string, "Entering directory") != NULL)
@xiota
pos = strstr(pos, "/"); // instead of ... pos = strstr(string, "/");
Sounds good! I'll update the PR. Thanks!
Are there any tangible benefits besides maybe saving a few processor cycles or avoiding some nested function call?
Why to not save maybe a few processor cycles or not avoid some nested function call? :)
@xiota commented on this pull request.
@@ -1057,7 +1057,7 @@ gboolean build_parse_make_dir(const gchar *string, gchar **prefix)
if (string == NULL) return FALSE;
- if ((pos = strstr(string, "Entering directory")) != NULL) + if (strstr(string, "Entering directory") != NULL)
Upon further reflection... the change I suggested rescans the "Entering directory" portion of the string... Should probably be (but haven't tested): ```C pos = strchr(pos + 18, '/'); ```
From search, the string looks like:
``` make[1]: Entering directory `/mnt/lfs/sources/gcc-build' ```
Upon further reflection... the change I suggested rescans the "Entering directory" portion of the string... Should probably be (but haven't tested): ```C pos = strchr(pos + 18, '/'); ```
From search, the string looks like:
``` make[1]: Entering directory `/mnt/lfs/sources/gcc-build' ```
pos = strchr(pos + 18, '/');
I guess we can use hard-coded values (`18`) as long as it's guaranteed that the string format is not going to change?
What do you think about `pos = strchr(pos + strlen("Entering directory") + 2, "/");` where 2 for space and `'`
Well... it's inside an `if` block, so the `pos` is guaranteed to point to the part of the string that starts with "Entering directory". ``` 12345678901234567890 Entering directory ``` I just tested the change with `+18`, and it captures the path correctly.
@rootkea pushed 1 commit.
1e6d602c0486302693cd29c35f52ec7e8c93d3ec Avoid re-scanning the string
@rootkea pushed 1 commit.
2e9617a52f79bbd170046917753f454ad99b5be9 Avoid re-scanning the string
The problem with `strlen("Entering directory '")` is `strlen` runs in linear time... basically equivalent to just rescanning the string. (Don't know whether compilers can optimize it to a constant.)
You're also including two chars ` '` that haven't been scanned yet, and it's the wrong quote char (`'` vs <code>`</code>)... so the latest push won't work anymore.
wrong quote char (' vs `)
Fixed now. Thanks!
The problem with strlen("Entering directory '") is strlen runs in linear time
I thought it could document the code well instead of using hard-coded number. But maybe I should add a commet instead and use the hard-coded number?
I thought it could document the code well instead of using hard-coded number. But maybe I should add a comment instead and use the hard-coded number?
According to internet, clang and gcc can optimize `strlen` to a constant. So I guess it would okay. I'd recommend leaving the space and quote out, since it's possible different implementations of make might use different quotes, especially on different operating systems.
@rootkea pushed 1 commit.
4294a1d33cef1a5bbc3bc3284db5d2d0ab14812a Avoid re-scanning the string
According to internet, clang and gcc can optimize strlen to a constant.
Yeah, but depends on what -O settings distros use.
Either way it is scanning 18 chars in a loop thats reading data via a pipe from another process, which involves system calls and possibly process switches ie the difference is immaterial. I would just let it rescan for simpler code.
@xiota commented on this pull request.
@@ -1057,13 +1057,13 @@ gboolean build_parse_make_dir(const gchar *string, gchar **prefix)
if (string == NULL) return FALSE;
- if ((pos = strstr(string, "Entering directory")) != NULL) + if ((pos = strstr(string, "Entering directory `")) != NULL)
Changing the string in the if-conditional could break something later.
@elextr commented on this pull request.
@@ -1057,13 +1057,13 @@ gboolean build_parse_make_dir(const gchar *string, gchar **prefix)
if (string == NULL) return FALSE;
- if ((pos = strstr(string, "Entering directory")) != NULL) + if ((pos = strstr(string, "Entering directory `")) != NULL)
Yeah, so just rescan from the start of "Enetring Directory" and it doesn't matter if the string changes.
@rootkea pushed 1 commit.
63253453e5f9249f46e4600920eea8b6e7007572 Use strchr instead of strstr
@rootkea commented on this pull request.
@@ -1057,13 +1057,13 @@ gboolean build_parse_make_dir(const gchar *string, gchar **prefix)
if (string == NULL) return FALSE;
- if ((pos = strstr(string, "Entering directory")) != NULL) + if ((pos = strstr(string, "Entering directory `")) != NULL)
Done!
@rootkea Can you copy/paste the error/warning message from `scan-build` gives? (Since that seems to be the main motivation for this change.)
The problem with `strlen("Entering directory '")` is `strlen` runs in linear time... basically equivalent to just rescanning (that portion of) the string. (Don't know whether compilers can optimize it to a constant.)
It looks like @elextr nixed the idea of moving the pointer ahead for this case, but for future reference, I think you could use `sizeof "Entering directory"` instead of strlen. Because it's a constant, sizeof can be used, then the number will be known at compile-time and won't need to be computed at run-time. (Sometimes it will be important to be mindful that using the `sizeof` operator will give the result of the string length + 1 for the NULL terminator.
@andy5995 commented on this pull request.
@@ -1063,7 +1063,7 @@ gboolean build_parse_make_dir(const gchar *string, gchar **prefix)
gchar *input;
/* get the start of the path */ - pos = strstr(string, "/"); + pos = strchr(pos, "/");
```suggestion pos = strchr(pos, '/'); ``` When using `strchr`, use single quotes around the char (because it's a char and not a string)
@rootkea pushed 1 commit.
77f4b5c295225ecb5b6cdcf1b19dc08b7736b345 Use strchr instead of strstr
@rootkea @xiota If you have any extra curiosity about fun with strings, you want to look at some of the functions in a couple of my small programs, such as [canfigger](https://github.com/andy5995/canfigger) or [rmw](https://remove-to-waste.info/). Keeping in mind I'm not a pro so at some later time you might find better ways to do things than I've done ;)
@rootkea commented on this pull request.
@@ -1063,7 +1063,7 @@ gboolean build_parse_make_dir(const gchar *string, gchar **prefix)
gchar *input;
/* get the start of the path */ - pos = strstr(string, "/"); + pos = strchr(pos, "/");
Thanks! Fixed now. :)
@rootkea Can you copy/paste the error/warning message from `scan-build` gives? (Since that seems to be the main motivation for this change.)
`scan-build` warned regarding redundant assignment to `pos` in `if` expression (line 1060) since on master we reassign to `pos` at line 1066 without reusing the earlier assigned value.
But now, we are passing the earlier assigned value of `pos` to `strchr()` as you suggested so assignment at line 1060 is no longer redundant.
@andy5995 that would still have the string constant in two places, if it really was worth doing do it like:
```c static char ed[] = "Entering Directory"; .. pos = strstr(string, ed); .. strchr(pos + sizeof ed - 1, '/') ```
Note the -1 to account for the end NUL.
@andy5995 Thanks for the pointers! I'll definitely take a look. :)
But as I said I don't think its worth doing.
@elextr , fwiw, I agree. ;)
github-comments@lists.geany.org