utils: fs, new memory filesystem #108
Conversation
| return fs.base | ||
| } | ||
|
|
||
| type file struct { |
There was a problem hiding this comment.
Maybe it's worth to move file, fileInfo and content to a different file.
There was a problem hiding this comment.
errr, I don't like, is everything is part of the same thing, and since you don't have test only for the files, is weird have a file.go without the file_test.go
| func (fs *Memory) OpenFile(filename string, flag int, perm os.FileMode) (fs.File, error) { | ||
| fullpath := fs.Join(fs.base, filename) | ||
| f, ok := fs.s.files[fullpath] | ||
| if !ok && flag&os.O_CREATE == 0 { |
There was a problem hiding this comment.
replace bit checks with functions:
- isCreate(flag)
- isNotCreate(flag)
- isAppend(flag)
- isNotAppend(flag)
- ...
There was a problem hiding this comment.
I really love this kind of abstractions, but this is more a general problem, not only related with this package.
| fs.BaseFile | ||
|
|
||
| c *content | ||
| p int64 |
There was a problem hiding this comment.
Use the full name for c and p, they are frequently used from quite far away.
| return newFileInfo(fs.base, fullpath, fs.s.files[filename].c.size), nil | ||
| } | ||
|
|
||
| info, err := fs.ReadDir(filename) |
There was a problem hiding this comment.
the name and the type of the variable info is misleading and its usage bellow is confusing (len(info)), it looks like the type is FileInfo, but it is actually []FileInfo. Can we choose a better name? (like infos, for example).
|
|
||
| info, err := fs.ReadDir(filename) | ||
| if err == nil && len(info) != 0 { | ||
| return newFileInfo(fs.base, fullpath, int64(len(info))), nil |
There was a problem hiding this comment.
What is the len(info) of an empty directory?
There was a problem hiding this comment.
A dir doesn't exists if is empty
| base = fs.Join(fs.base, base) | ||
|
|
||
| dirs := make(map[string]bool, 0) | ||
| for fullpath, f := range fs.s.files { |
There was a problem hiding this comment.
This operation is O(n) with n being the number of files in the file system. Why don't we use trees instead of maps?
|
|
||
| func (c *content) WriteAt(p []byte, off int64) (int, error) { | ||
| l := len(p) | ||
| if int(off)+l > len(c.bytes) { |
There was a problem hiding this comment.
Shouldn't we be checking the capacity of c.bytes instead of the length?
| func (c *content) WriteAt(p []byte, off int64) (int, error) { | ||
| l := len(p) | ||
| if int(off)+l > len(c.bytes) { | ||
| buf := make([]byte, 2*len(c.bytes)+l) |
There was a problem hiding this comment.
why just nor grow buf until len(c.bytes) + l?
| if off+int64(n) > c.size { | ||
| c.size = off + int64(n) | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't understand where are we setting the length of c.bytes and why are we ignoring the result of copy.
| l = c.size - off | ||
| } | ||
|
|
||
| n := copy(b, c.bytes[off:l]) |
There was a problem hiding this comment.
This cannot be right, l can be less than off here, but that is clearly impossible. I think you have to change c.bytes[off:l] to c.bytes[off:off+l]. I think there has been mistake testing this code, what was its test coverage?
There was a problem hiding this comment.
fixes and coverage improved
| } | ||
|
|
||
| func (c *content) Truncate() { | ||
| c.bytes = []byte{} |
| @@ -39,7 +40,7 @@ func (fs *Memory) Open(filename string) (fs.File, error) { | |||
| func (fs *Memory) OpenFile(filename string, flag int, perm os.FileMode) (fs.File, error) { | |||
| fullpath := fs.Join(fs.base, filename) | |||
| f, ok := fs.s.files[fullpath] | |||
| if !ok && flag&os.O_CREATE == 0 { | |||
| if !ok && !isCreate(flag) { | |||
There was a problem hiding this comment.
you are not checking if flag is O_CREATE but if flags has O_CREATE set. I would rename all the check functions to hasFoo: hasCreate(flag), hasAppend(flag)... And I would create the negate ones also: hasNotCreate(flag), hasNotAppend(flag)...
| } | ||
|
|
||
| return | ||
| } | ||
|
|
||
| func (fs *Memory) TempFile(dir, prefix string) (fs.File, error) { | ||
| filename := fmt.Sprintf("%s_%d", prefix, time.Now().UnixNano()) | ||
| fs.tempCount++ | ||
| filename := fmt.Sprintf("%s_%d_%d", prefix, fs.tempCount, time.Now().UnixNano()) |
There was a problem hiding this comment.
This does not solve the problem.
What is the purpose of TempFile()? If its purpose is to create a new file, then a new file has to be created, instead of truncating a file with a highly uncommon name. Also explain what this method is supposed to do and why it works differently than the standard TempFile method.
| return | ||
| } | ||
|
|
||
| buf := make([]byte, 2*len(c.bytes)+l) |
There was a problem hiding this comment.
Don't use length and capacity interchangeably.
Even if in this code they always have the same value, they represent quite different things. Choose one and stick to it. The one you are looking for is capacity, not length.
| @@ -298,12 +281,7 @@ type content struct { | |||
| } | |||
|
|
|||
| func (c *content) WriteAt(p []byte, off int64) (int, error) { | |||
There was a problem hiding this comment.
There is no need for grow or copy here, just use the append function provided by the language. And remove size, this is what the length of a slice is for. You are rewriting here all the internal code of a slice.
| return n, nil | ||
| } | ||
|
|
||
| func (c *content) Truncate() { | ||
| c.bytes = []byte{} | ||
| c.bytes = make([]byte, 0) |
There was a problem hiding this comment.
Do we really want to throw away the preallocated buffer when truncating, we are wasting resources here.
* utils: fs, new memory filesystem * utils: fs, renamed os.NewOS to os.New * utils: fs, memory changes requested by @alcortes
This is a new
fs.Filesytembased on memory. take notice of is not thread-safe.Also I renamed os.NewOS
toos.New`