FileSystem access should go via IFileSystem (+refactoring) #11112
Conversation
…tememory (+streams) instead of whole bunch of apis
vzarytovskii
changed the title
|
Oops, accidently got rid of some fsi files, going to fix it... |
…writing files, just use FileStream for now instead of mmaped
| @@ -3990,27 +3990,153 @@ FSharp.Compiler.EditorServices.XmlDocable: Int32 line | |||
| FSharp.Compiler.EditorServices.XmlDocable: Microsoft.FSharp.Collections.FSharpList`1[System.String] get_paramNames() | |||
| FSharp.Compiler.EditorServices.XmlDocable: Microsoft.FSharp.Collections.FSharpList`1[System.String] paramNames | |||
| FSharp.Compiler.EditorServices.XmlDocable: System.String ToString() | |||
| FSharp.Compiler.IO.ByteArrayMemory | |||
There was a problem hiding this comment.
Are these now public? They should be internal only.
There was a problem hiding this comment.
Since we expose ByteMemory type now via the IFileSystem interface, I was thinking to make ByteArrayMemory (and maybe other ones) public too, as default implementations, so FCS consumers can use it in their IFileSystem implementations. WDYT?
There was a problem hiding this comment.
ByteMemory and friends should really be internal if possible.
There was a problem hiding this comment.
@vzarytovskii and I talked a bit, we will expose these types but will rename them with the FSharp prefix and will be given experimental attributes as we might make them internal again and/or change them as we continue to iterate on this.
…s - only use mmaped files to read big binary files (i.e. in the ilreader)
| type IFileSystem = | ||
| abstract AssemblyLoader: IAssemblyLoader | ||
|
|
||
| abstract OpenFileForReadShim: filePath: string * ?useMemoryMappedFile: bool * ?shouldShadowCopy: bool -> ByteMemory |
There was a problem hiding this comment.
Not sure whether useMemoryMappedFile needs to be in the signature or not since it is the implementation details.
But we somehow need to indicate if we want to use mmap or not (we only want to use it for big files, it does not make sense to use it for source files since it locks the file and we need to either reuse the map when accessing from another process or shadow copy all source files).
There was a problem hiding this comment.
We discussed this a bit today. I think it's fine that you can't open an already memory-mapped file if it's not shadow copied; we expect that.
We will iterate more on this when we need to, but otherwise it looks good.
…it's a subject to change.
There was a problem hiding this comment.
This is really good work. I love that we are consolidating all of the file system related calls into one place. I know we will iterate more on this, but what we have here looks good.
One thing to note, in the future if we could not have as many formatting changes that would help the reviewing process.
Yeah, it was already too late when I noticed that my editor did all those changes, sorry :( |
vzarytovskii
changed the title
This is a work in progress change, will be in which all FS work should go via the IFileSystem.
I've decided to make changes in smaller batches, to minimize the area: