Skip to content

use IS_SLASH in various places instead of string comparing#21814

Closed
LamentXU123 wants to merge 5 commits intophp:masterfrom
LamentXU123:refactor-8
Closed

use IS_SLASH in various places instead of string comparing#21814
LamentXU123 wants to merge 5 commits intophp:masterfrom
LamentXU123:refactor-8

Conversation

@LamentXU123
Copy link
Copy Markdown
Contributor

@LamentXU123 LamentXU123 commented Apr 20, 2026

we have a defined IS_SLASH function:

#define IS_SLASH(c)	((c) == '/' || (c) == '\\')

So to make things more legible we can use IS_SLASH(XXX) instead of XXX == '\\' || XXX == '/'

I search all occasions by this command:

rg -n "'/'.*'\\'|'\\'.*'/'|== '/' \|\| .*== '\\'|!= '/' && .*!= '\\'|== '\\' \|\| .*== '/'|!= '\\' && .*!= '/'"

and manually remove some improper occasions :)

Copy link
Copy Markdown
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You missed some, for example

if (next_char == '/' || next_char == '\\' || next_char == '\0') {

Please do the search with a robust tool, not with regular expressions. The following Coccinelle patch can be a good start:

@@
char x;
@@

* (x == '/' || x == '\\')

@@
char x;
@@

* (x == '\\' || x == '/')

@LamentXU123
Copy link
Copy Markdown
Contributor Author

You missed some, for example

if (next_char == '/' || next_char == '\\' || next_char == '\0') {

Please do the search with a robust tool, not with regular expressions. The following Coccinelle patch can be a good start:

No this is intented... If we change this the test on ext/phar/tests/bug64931/bug64931.phpt will fail. See https://github.com/php/php-src/actions/runs/24648588020/job/72066292649 as I've said I manually deleted some changes

The following Coccinelle patch can be a good start:

Yeah I'll try now, thank you :)

Comment thread ext/standard/string.c Outdated
@TimWolla
Copy link
Copy Markdown
Member

If we change this the test on ext/phar/tests/bug64931/bug64931.phpt will fail

That's not how C works.

@iluuu1994
Copy link
Copy Markdown
Member

This changes behavior, no? IS_SLASH() is defined as #define IS_SLASH(c) ((c) == '/') on non-Windows systems.

@TimWolla
Copy link
Copy Markdown
Member

IS_SLASH() is defined as #define IS_SLASH(c) ((c) == '/') on non-Windows systems.

Ah, that explains the observed test failure then. And yes, this means that this PR is not a simple cleanup.

@iluuu1994
Copy link
Copy Markdown
Member

Right, so I think this PR is inherently problematic. Closing, but thanks anyway.

@iluuu1994 iluuu1994 closed this Apr 20, 2026
@LamentXU123
Copy link
Copy Markdown
Contributor Author

IS_SLASH() is defined as #define IS_SLASH(c) ((c) == '/') on non-Windows systems.

Ah, that explains the observed test failure then. And yes, this means that this PR is not a simple cleanup.

Uh that explains test failure. Then good to close this, and thank for reviewing anyways :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants