Fix extract from stream #103
Conversation
| Identifiers.Remove(_uniqueID); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm not 100% sure what the use of UniqueID is, while this implementation seems faster, it's also limited to ~4 billion instances where the previous implementation was not
Can you shed some light on this?
SevenZip/SevenZipExtractor.cs
Outdated
| @@ -107,7 +107,7 @@ private void Init(Stream stream) | |||
| SevenZipLibraryManager.LoadLibrary(this, _format); | |||
| try | |||
| { | |||
| _inStream = new ArchiveEmulationStreamProxy(stream, _offset); | |||
| _inStream = new ArchiveEmulationStreamProxy(stream, _offset, false); | |||
There was a problem hiding this comment.
Maybe make the last parameter optional (dispose = false) to make the API backward compatible?
Fix exctract from stream
fix missprint
fix tests result folder
|
Sorry for the silence, this was quite a big PR and a little hard to follow. In general I appreciate the effort and it seems mostly fine, but I'll add some questions as review comments. |
| @@ -0,0 +1,35 @@ | |||
| name: .NET Core | |||
There was a problem hiding this comment.
The inclusion of a workflows config is a bit surprising. What is the reasoning for this?
There was a problem hiding this comment.
I have add it to find out why some of SevenZipCompressorTests.cs tests failed at AppVeyor
but then I fix tests at a5d0dd4. If you do not plan to use github workflow for tests can delete it
| @@ -1,6 +1,6 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
| <PropertyGroup> | |||
| <TargetFrameworks>netstandard2.0;net45</TargetFrameworks> | |||
| <TargetFrameworks>netcoreapp2.1;net45;net462</TargetFrameworks> | |||
There was a problem hiding this comment.
Why is this target changed from .NET Standard 2.0 to Core 2.1, while the SevenZip project itself remains Standard 2.0?
There was a problem hiding this comment.
it is only one way how I can run tests with not net45 and net462 frameworks
I found it at https://github.com/nunit/docs/wiki/.NET-Core-and-.NET-Standard#why-cant-my-tests-target-net-standard
| /// Use SevenZipExtractor(string) to extract from disk, though it is not necessary.</param> | ||
| /// <remarks>The archive format is guessed by the signature.</remarks> | ||
| public SevenZipExtractor(Stream archiveStream) | ||
| public SevenZipExtractor(Stream archiveStream, bool leaveOpen = false) |
There was a problem hiding this comment.
Pretty much just a suggestion, but wouldn't it be better to reverse this parameter - bool disposeStream = true?
Since the use of leaveOpen downstream is always if (!leaveOpen) it would make for easier reading of the code, in my opinion at least.
The param comment would then be something like Whether or not to dispose the stream after operation is complete..
There was a problem hiding this comment.
I have change it after @vasily-kirichenko review to make it more like System.IO.StreamReader
| Identifiers.Remove(_uniqueID); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm not 100% sure what the use of UniqueID is, while this implementation seems faster, it's also limited to ~4 billion instances where the previous implementation was not
Can you shed some light on this?
Changes:
fix unexpected behavior:
SevenZipExtractor dispose archiveStream when created by public SevenZipExtractor(Stream archiveStream)
fix perfomance degradation at SevenZipBase.GetUniqueID()
under high load Identifiers list's length increases upto 80000 and when new object created all cpu time spended at SevenZipBase.GetUniqueID()
add net462 target - fupported long paths by UNC (\\?\disk:\folder\file)
fixed netstandard2.0 target in test projects