Use strchr instead of strstr

[rootkea]

Found by: scan-build

[xiota]

What about this, to avoid rescanning the beginning of the string:

if ((pos = strstr(string, "Entering directory")) != NULL)
{
	// ...

	pos = strstr(pos, "/");   // instead of ... pos = strstr(string, "/");

	// ...
}

[andy5995]

And is there any reason not to use strchr instead of strstr (when searching for '/')?

[xiota]

@andy5995 Are there any tangible benefits besides maybe saving a few processor cycles or avoiding some nested function call?

[rootkea]

@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]

Upon further reflection... the change I suggested rescans the "Entering directory" portion of the string... Should probably be (but haven't tested):

pos = strchr(pos + 18, '/');

From search, the string looks like:

make[1]: Entering directory `/mnt/lfs/sources/gcc-build'

[rootkea]

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?

[rootkea]

What do you think about pos = strchr(pos + strlen("Entering directory") + 2, "/"); where 2 for space and '

Edit - OR simply pos = strchr(pos + strlen("Entering directory '"), "/");

[xiota]

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.

[xiota]

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.)

You're also using the wrong quote char (' vs `)... so the latest push won't work anymore.

[rootkea]

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 comment instead and use the hard-coded number?

[xiota]

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.

[elextr]

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]

@rootkea Can you copy/paste the error/warning message from scan-build gives? (Since that seems to be the main motivation for this change.)

[andy5995]

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]

@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 or rmw. 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]

@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 using 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.

[elextr]

@andy5995 that would still have the string constant in two places, if it really was worth doing do it like:

static char ed[] = "Entering Directory";
.
pos = strstr(string, ed);
.
strchr(pos + sizeof ed - 1, '/')

Note the -1 to account for the end NUL.

But as I said I don't think its worth doing.

[rootkea]

@andy5995 Thanks for the pointers! I'll definitely take a look. :)

[andy5995]

But as I said I don't think its worth doing.

@elextr , fwiw, I agree. ;)