Use strlen(str) instead of sizeof(str)-1 for directly declared strings

[mvorisek]

Modernize & unify code to the prefered variant [1]

regex replace used:

• from sizeof(\s*\(\s*"[^"\\]*"\s*\))\s*-\s*1(?!\w)
• to strlen$1

I firstly replaced all *.h and *.c files and then carefully reviewed the rest files.

[1] #8241 (comment)

[TimWolla]

regex replace used:

Tip for this type of replacement: Use Coccinelle.

$ cat test.c
#include <string.h>

struct test {
	int a;
	int b;
}

void foo(char *buf, size_t len);
void bar(size_t bar);

int
main(void) {
	const char *foo = "foo";

	foo("foo", sizeof("foo") - 1);
	foo("foo", sizeof("foo"));
	foo(foo, sizeof(foo));

	bar(sizeof(struct test) - 1);
}
$ cat test.cocci 
@@
constant char[] s;
@@

- (sizeof(s) - 1)
+ strlen(s)
$ spatch -sp_file test.cocci test.c
init_defs_builtins: /usr/bin/../lib/coccinelle/standard.h
HANDLING: test.c
diff = 
--- test.c
+++ /tmp/cocci-output-3782-b347f1-test.c
@@ -12,7 +12,7 @@ int
 main(void) {
 	const char *foo = "foo";
 
-	foo("foo", sizeof("foo") - 1);
+	foo("foo", strlen("foo"));
 	foo("foo", sizeof("foo"));
 	foo(foo, sizeof(foo));

[mvorisek]

- (sizeof(s) - 1)
+ strlen(s)

@TimWolla this PR aims to replace only the "directly declared strings" (like "string") and only if not used within case and {} array initialization (old compilers does not support strlen there), if you know a good tool to define such replaces based on C AST, please let me know and we can add it to #8295 to assert the code quality

[iluuu1994]

I'm a bit worried about merge conflicts when merging upstream. We never removed the {{{ comments for the same reason. Although the impact there was surely bigger.

[TimWolla]

@mvorisek Coccinelle 😃. The patch gets a little more complicated, though.

$ cat test.c; echo; echo
#include <stdio.h>
#include <string.h>

struct test {
	int a;
	int b;
};

void foo(const char *buf, size_t len) { }
void bar(size_t bar) { }

int
main(void) {
	const char *f = "foo";

	foo("foo", sizeof("foo") - 1);
	foo("foo", sizeof("foo"));
	foo(f, sizeof(f));

	bar(sizeof(struct test) - 1);
}

void
array_init() {
	size_t arr[] = { sizeof("foo") - 1, sizeof("bar") - 1 };
}

void
switch_case(size_t i) {
	switch (i) {
		case 1:
			break;
		case 2:
		case sizeof("foo") - 1:
			printf("%lu\n", sizeof("foo") - 1);
			break;
		case 5:
			break;
	}
}


$ cat test.cocci; echo; echo
@r_case@
position p;
expression e;
@@

switch (...)
{
case sizeof(e)@p - 1: ...
}

@r_array_init@
type T;
identifier arr;
expression e;
position p;
@@

T arr[] = {
...,
sizeof(e)@p - 1,
...
};

@@
constant char[] s;
position p != { r_case.p, r_array_init.p };
@@

- (sizeof(s)@p - 1)
+ strlen(s)


$ spatch -sp_file test.cocci test.c
init_defs_builtins: /usr/bin/../lib/coccinelle/standard.h
HANDLING: test.c
diff = 
--- test.c
+++ /tmp/cocci-output-3942-8f1e09-test.c
@@ -13,7 +13,7 @@ int
 main(void) {
 	const char *f = "foo";
 
-	foo("foo", sizeof("foo") - 1);
+	foo("foo", strlen("foo"));
 	foo("foo", sizeof("foo"));
 	foo(f, sizeof(f));
 
@@ -32,7 +32,7 @@ switch_case(size_t i) {
 			break;
 		case 2:
 		case sizeof("foo") - 1:
-			printf("%lu\n", sizeof("foo") - 1);
+			printf("%lu\n", strlen("foo"));
 			break;
 		case 5:
 			break;

[Girgias]

The arg info files are generated files, however the script generating them is not updated.

[bwoebi]

Interestingly MSVC does not optimize strlen("str") to 3 in its -O0 config, while clang and gcc do. I assume that's acceptable on Windows maybe though?

[cmb69]

Interestingly MSVC does not optimize strlen("str") to 3 in its -O0 config, while clang and gcc do. I assume that's acceptable on Windows maybe though?

Yeah, I think this is acceptable; a lot of other important optimizations are not done with /O0. And we do not do any "official" debug builds for Windows anyway (neither CI nor elsewhere).

[Girgias]

Either keep the sizeof here, or I would imagine using strlen for the line above, but this looks a bit weird and might be done for good reasons

[Girgias]

Note: but these usages of SUCCESS should probably be removed in favour of using 0 explicitly.

[derickr]

This seems like making changes just for the sake of making changes, so closing it.

[mvorisek]

I refactored code to the prefered variant which is more readable and shorter. Is it a bad practise? I was planning to add Coccinelle script so the prefered variant is asserted by CI, so even merging issues are easily fixable. Can this PR be reopened?

[mvorisek]

regex replace used:

Tip for this type of replacement: Use Coccinelle.

@TimWolla do you think Coccinelle can be used to transform spaces indentation to tabs?

https://github.com/php/php-src/blob/master/.github/actions/verify-generated-files/action.yml#L15 can then be used for diff output