-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Virtual cwd refactoring #15524
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Virtual cwd refactoring #15524
Conversation
d74be08 to
01b27a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cmb69 as don't have a Windows machine and can't test it, is such a function call change even sensible or not?
And if yes, can I also provide a size_t pointer for the 3rd argument so that PHP_WIN32_IOUTIL_CHECK_PATH_W() doesn't need to make a call to wstrlen()?
As seemingly this changes the behaviour of some tests in ways that I don't totally understand.
01b27a3 to
95d2fc5
Compare
95d2fc5 to
5194bd9
Compare
I'm not sure whether doing this change in zend_virtual_cwd.c now is a good idea. According to "Make it work, make it right, make it fast", we shouldn't skip the second step. That code is really contrieved, and especially tsrm_realpath_r() is a 500 line long mess. Some "extract functions" refactorings seem to be in order, although that doesn't appear to be simple; both Anatol and I tried, but gave up, a couple of years ago; that doesn't mean that it's not worth to try a fresh attempt.
I'm not sure whether doing this change in zend_virtual_cwd.c now is a good idea. According to "Make it work, make it right, make it fast", we shouldn't skip the second step. That code is really contrieved, and especially
tsrm_realpath_r()is a 500 line long mess. Some "extract functions" refactorings seem to be in order, although that doesn't appear to be simple; both Anatol and I tried, but gave up, a couple of years ago; that doesn't mean that it's not worth to try a fresh attempt.
I'll chip at this when I have motivation, as passing only a char* is far from ideal.
Uh oh!
There was an error while loading. Please reload this page.
Commits should be reviewed individually.
Motivation
There are two primary motivations:
strlen()when we already know itintto convey meaning and intentTo achieve this, refactoring is conducted one step at a time on the various layers that interact with each other:
Some other/unrelated minor refactorings are likely going to happen in some commits when I encounter some oddities.