new implementation for glob()#1
Draft
diegoiast wants to merge 24 commits into
Draft
Conversation
It seems that serenityOS has no `glob()`
support. I am unsure if this is by design, or not needed yet, or is it not needed in Posix.
The Win/DOS glob are borken, so this implementation will replace them soon. Problems in `dosglob()`:
1. `globfree()` did not free `glob->gl_pathv`
2. when `insert()` failed sometimes due to memory issues,
and this was not propagated up - and lead to garbage
on `dir`.
3. Before `findnext()` in dos-glob.c, the entries count was broken.
4. The code creates a very large string array - and then copies all
the strings back to `pglob->gl_pathv[]`, while it could just
move pointers and save `memcpy()` and reduce memory consumption
(and CPU time!)
Doing this a new file, and only implementing the `opendir()`/
`findfirst()` and then iterating over dirs - this is the only platform
dependent code - which means valgrind will find memory issues.
How about `pglob()` like...portable glob? or `sglob()`, like in
sereniry glob?
Win32 implementation is working. Code was very simple. I also found out that the Posix version is broken, so, I will need to fix it before I go to the DOS version.
As I am looking "way too much" into SerenityOS development, and I look up to the ideals (and this code will help the port...) I am keeping the SerenityGlob naming :) This has been moved to the global library, as it is not dependent on platform. Next - port to DOS !
This was not that hard. The main problem was finding simple compilation errors. This cross platform model is begining to deliver - and I like it.
1. all code uses serenity code, all platforms + compilers. 2. Fix OW Makefile to build sglob.c 3. guard another definition inside `sglob.h`
1. all code uses serenity code, all platforms + compilers. Old glob files are gone. 2. Fix OW Makefile to build sglob.c 3. guard another definition inside `sglob.h`
CLion+tidy can find a lot of small problems. Wow!
It seems that on some DOS compilation models, this is defined differently. We already have it in stdlib or something.
We already have this large buffer for reading the main line, but not using for computing the prompt? The memory is available. Also - use the same definition for header/source of `command_execute_line`
Command parsing is needed only for checking version, and other stuff. In all other cases, all that structure is not needed, and we can avoid allocating it. Making this as part of the `if else` saves us some bytes.
Use `sglob()` on all platforms (unhappy about it, will rever this soon). Serenity port compiles!
On serenity, without this - we don't see the prompt. Seems like a good idea anyway.
Now we can `dir` inside SerenityOS. It seems that `fnmatch()` is not implemented - and also `glob()`, so our abstranctions are in place. It seems like bash and dash were prepared for this, and the internal SerenityOS shell does not use this codepath at all, but classes/functions inside `AK::`.
I am abusing this makefile for SerenityOS anyway. I am not pushing the hardcoded hacks yet, need better solutions.
diegoiast
force-pushed
the
glob-rewrite
branch
from
January 19, 2022 10:13
94e6005 to
950c98b
Compare
OpenWatcom2 start usig *.xz instead of *.gz for the releases - which broke our CI/CD. This commit fixes it.
Apparently, I don't know how to use this API. Fixed it - and not it does the basic thing. However - many small test cases fail: for example, `dir /` will fail. I will investigate it later on.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It seems that serenityOS has no
glob()support. I am unsure if this is by design, or not needed yet, or is it not needed in Posix.
The Win/DOS glob are borken, so this implementation will replace them soon. Problems in
dosglob():globfree()did not freeglob->gl_pathvinsert()failed sometimes due to memory issues,and this was not propagated up - and lead to garbage
on
dir.findnext()in dos-glob.c, the entries count was broken.the strings back to
pglob->gl_pathv[], while it could justmove pointers and save
memcpy()and reduce memory consumption(and CPU time!)
Doing this a new file, and only implementing the
opendir()/findfirst()and then iterating over dirs - this is the only platformdependent code - which means valgrind will find memory issues.
How about
pglob()like...portable glob? orsglob(), like insereniry glob?