fix #1683: инкрементальное ХешированиеДанных без захвата файлов#1684
Open
Mr-Rm wants to merge 7 commits into
Open
fix #1683: инкрементальное ХешированиеДанных без захвата файлов#1684Mr-Rm wants to merge 7 commits into
Mr-Rm wants to merge 7 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCRC32 fixes byte-range processing and HashFinal output, and gains AppendData/GetCurrentHash APIs for incremental use. HashImpl is refactored to use IncrementalHash with an optional Crc32 accumulator; append and file/stream handling were rewritten. Tests add file-stream and incremental CRC32 checks. ChangesIncremental hashing support
Sequence Diagram(s) sequenceDiagram
participant Caller
participant HashImpl
participant IncrementalHash
participant Crc32
participant FileStream
Caller->>HashImpl: Append(data, count)
alt CRC32 mode
HashImpl->>Crc32: AppendData(bytes)
Crc32-->>HashImpl: updated CRC state
else non-CRC32
HashImpl->>IncrementalHash: AppendData(bytes)
IncrementalHash-->>HashImpl: updated hash bytes
end
Caller->>HashImpl: Hash()/HashString()
HashImpl-->>Caller: current hash bytes or CRC32 UInt32
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/OneScript.StandardLibrary/Hash/Crc32.cs`:
- Around line 83-85: AppendData(byte[] array, int offset, int count) is passing
count into HashCore while HashCore treats its third parameter as an end index
(cbSize), so non-zero offsets cause skipped/truncated bytes; fix by passing the
correct end index to HashCore (i.e., call HashCore(array, offset, offset +
count)) or alternatively change HashCore to accept a count and update calls
accordingly—ensure the methods referenced (AppendData and HashCore) iterate from
offset up to offset+count rather than treating count as an absolute end index.
In `@src/OneScript.StandardLibrary/Hash/HashImpl.cs`:
- Around line 61-72: The HashString getter can NRE when _hash is null; update
the HashString property (in HashImpl.cs) to mirror the uninitialized-state
handling used by the Hash property: after the _crc32 check, return an
appropriate empty/default string if _hash == null (instead of entering the
for-loop), otherwise proceed to build the hex string from _hash; reference
_crc32, _hash, HashString, and AppendData to locate and align the logic with the
existing Hash behavior.
- Around line 153-155: The FileStream created in HashImpl.cs is not disposed if
AppendStream(stream) throws, causing a file handle leak; wrap the FileStream
creation in a using or try/finally so the stream is disposed regardless of
exceptions (replace the current FileStream + AppendStream(stream);
stream.Close() pattern with a using(var stream = new FileStream(...)) {
AppendStream(stream); } or equivalent) to ensure the stream is always closed on
success or error.
- Around line 126-138: Append currently accepts negative count (Append(BslValue
toAdd, int count = 0)) which lets callers reach AppendStream(Stream, int) and
cause CLR exceptions when allocating buffers; add an explicit check for count <
0 at the start of Append (and/or at the top of AppendStream) and raise the
appropriate script-level argument error (instead of letting a negative value
flow into new byte[Math.Min(BUFFER_SIZE, count)]). Reference symbols:
Append(BslValue toAdd, int count = 0), AppendStream(Stream, int), and
BUFFER_SIZE.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff10e8e4-a66b-43d1-973c-daa1e5f9857f
📒 Files selected for processing (3)
src/OneScript.StandardLibrary/Hash/Crc32.cssrc/OneScript.StandardLibrary/Hash/HashImpl.cstests/hash.os
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OneScript.StandardLibrary/Hash/Crc32.cs (1)
78-86: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd input validation to match standard .NET API behavior.
The
AppendDatamethods should validate their inputs to prevent unexpected exceptions and align with the behavior of similar .NET APIs likeIncrementalHash.AppendData, which validates arguments and throwsArgumentNullExceptionandArgumentOutOfRangeExceptionas appropriate.🛡️ Proposed fix to add input validation
public void AppendData(byte[] array) { + if (array == null) + throw new ArgumentNullException(nameof(array)); HashCore(array, 0, array.Length); } public void AppendData(byte[] array, int offset, int count) { + if (array == null) + throw new ArgumentNullException(nameof(array)); + if (offset < 0 || offset > array.Length) + throw new ArgumentOutOfRangeException(nameof(offset)); + if (count < 0 || count > array.Length - offset) + throw new ArgumentOutOfRangeException(nameof(count)); HashCore(array, offset, count); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/Hash/Crc32.cs` around lines 78 - 86, The AppendData(byte[] array) and AppendData(byte[] array, int offset, int count) methods lack argument validation; add checks so they mirror .NET behavior: in both overloads throw ArgumentNullException when array is null; in the (array, offset, count) overload validate that offset and count are non-negative and that offset + count does not exceed array.Length and throw ArgumentOutOfRangeException for violations; after these checks call HashCore(array, offset, count) (the zero-arg overload can forward to the validated overload or validate then call HashCore(array, 0, array.Length)).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/OneScript.StandardLibrary/Hash/Crc32.cs`:
- Around line 78-86: The AppendData(byte[] array) and AppendData(byte[] array,
int offset, int count) methods lack argument validation; add checks so they
mirror .NET behavior: in both overloads throw ArgumentNullException when array
is null; in the (array, offset, count) overload validate that offset and count
are non-negative and that offset + count does not exceed array.Length and throw
ArgumentOutOfRangeException for violations; after these checks call
HashCore(array, offset, count) (the zero-arg overload can forward to the
validated overload or validate then call HashCore(array, 0, array.Length)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf1dedca-b810-430f-96b5-a7577dfa90e9
📒 Files selected for processing (1)
src/OneScript.StandardLibrary/Hash/Crc32.cs
Collaborator
Author
|
К замечанию @coderabbitai после 1a2ad44: класс |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
Своё замечание снимаю. 🐇 🏁 Script executed: Length of output: 210 🏁 Script executed: Length of output: 0 Замечание, по всей видимости, уже было разрешено или находилось в рамках проверки, недоступной через текущий API. В любом случае, пояснение принято, обоснование учтено — ✏️ Learnings added
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OneScript.StandardLibrary/Hash/HashImpl.cs (1)
47-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmpty-input hashes regressed to sentinel values.
For untouched MD5/SHA instances,
HashSumnow returns numeric0andHashSumOfStringreturns"0"instead of the digest of the empty byte sequence. This also formats empty CRC32 inconsistently before the first append.♻️ Proposed fix
public IValue Hash { get { - if(!_calculated) - return ValueFactory.Create(0); - if (_crc32 != null) - return ValueFactory.Create(_crc32.GetCurrentHashAsUInt32()); - - return new BinaryDataContext(_hash); + return ValueFactory.Create(_crc32.GetCurrentHashAsUInt32()); + + return new BinaryDataContext(_hash ?? _provider.GetCurrentHash()); } } @@ public string HashString { get { - if (!_calculated) - return "0"; - if (_crc32 != null) return _crc32.GetCurrentHashAsUInt32().ToString("X8"); + var hash = _hash ?? _provider.GetCurrentHash(); var sb = new StringBuilder(); - for (int i = 0; i < _hash.Length; i++) - sb.Append(_hash[i].ToString("X2")); + for (int i = 0; i < hash.Length; i++) + sb.Append(hash[i].ToString("X2")); return sb.ToString(); } }Also applies to: 67-76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/Hash/HashImpl.cs` around lines 47 - 58, The Hash/HashSum regressions happen because when _calculated is false the code returns sentinel zeros instead of the actual digest of an empty input; update the Hash property and the related HashSumOfString logic (also the block handling lines 67-76) to compute and return the digest for an empty byte sequence when no data has been appended: for CRC32 use _crc32.GetCurrentHashAsUInt32() (or call the CRC32 routine on an empty buffer) and return that numeric value via ValueFactory.Create, and for MD5/SHA return a BinaryDataContext constructed from the computed digest bytes (use _hash or compute hash of Array.Empty<byte>() if _hash is empty) rather than returning 0 or "0"; ensure this logic references _calculated, _crc32, _hash, Hash, and HashSumOfString so untouched instances produce the correct empty-input digest.
🧹 Nitpick comments (1)
src/OneScript.StandardLibrary/Hash/HashImpl.cs (1)
153-159: ⚡ Quick winGuard
pathexplicitly beforeFile.Exists.
File.Exists(path)collapsesnullinto the same branch as “file does not exist”, so direct C# callers lose a clear null-contract failure. A fast null guard keeps script-side empty strings working while making direct calls deterministic.Based on learnings Guideline: implement defensive null checks for these parameters in public methods that can be called from scripts.♻️ Proposed fix
public void AppendFile(string path) { + if (path == null) + throw new ArgumentNullException(nameof(path)); + if (!File.Exists(path)) throw RuntimeException.InvalidArgumentType(); using var stream = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite); AppendStream(stream); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/Hash/HashImpl.cs` around lines 153 - 159, The method AppendFile should explicitly guard against a null path before calling File.Exists; add a null check at the start of AppendFile and throw RuntimeException.InvalidArgumentType() when path == null, then continue to the existing File.Exists(path) check (do not treat empty string as null so script-side empty strings still follow existing behavior); this change touches the AppendFile method and leaves subsequent FileStream and AppendStream calls unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/OneScript.StandardLibrary/Hash/HashImpl.cs`:
- Around line 47-58: The Hash/HashSum regressions happen because when
_calculated is false the code returns sentinel zeros instead of the actual
digest of an empty input; update the Hash property and the related
HashSumOfString logic (also the block handling lines 67-76) to compute and
return the digest for an empty byte sequence when no data has been appended: for
CRC32 use _crc32.GetCurrentHashAsUInt32() (or call the CRC32 routine on an empty
buffer) and return that numeric value via ValueFactory.Create, and for MD5/SHA
return a BinaryDataContext constructed from the computed digest bytes (use _hash
or compute hash of Array.Empty<byte>() if _hash is empty) rather than returning
0 or "0"; ensure this logic references _calculated, _crc32, _hash, Hash, and
HashSumOfString so untouched instances produce the correct empty-input digest.
---
Nitpick comments:
In `@src/OneScript.StandardLibrary/Hash/HashImpl.cs`:
- Around line 153-159: The method AppendFile should explicitly guard against a
null path before calling File.Exists; add a null check at the start of
AppendFile and throw RuntimeException.InvalidArgumentType() when path == null,
then continue to the existing File.Exists(path) check (do not treat empty string
as null so script-side empty strings still follow existing behavior); this
change touches the AppendFile method and leaves subsequent FileStream and
AppendStream calls unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eaf0c623-68ff-4b98-a5a8-82323cfff910
📒 Files selected for processing (1)
src/OneScript.StandardLibrary/Hash/HashImpl.cs
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OneScript.StandardLibrary/Hash/HashImpl.cs (1)
22-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResource leak:
IncrementalHashis never disposed.
HashImplno longer implementsIDisposable, but the_providerfield is anIncrementalHash, which holds unmanaged cryptographic resources and requires deterministic disposal. With the previousDispose()removed, the native handle survives until the GC finalizer runs, which is non-deterministic and can lead to handle exhaustion or memory pressure for scripts that create manyHashImplinstances.Please restore
IDisposableonHashImpl(or another lifecycle hook recognized by the script context) and dispose_providerfrom it.♻️ Suggested fix
- public class HashImpl : AutoContext<HashImpl> + public class HashImpl : AutoContext<HashImpl>, IDisposable { ... + public void Dispose() + { + _provider?.Dispose(); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/Hash/HashImpl.cs` around lines 22 - 42, HashImpl currently holds an IncrementalHash in the _provider field but no longer implements IDisposable, leaking native handles; restore deterministic disposal by making HashImpl implement IDisposable (or the script context lifecycle interface used by AutoContext), add a Dispose/Dispose(bool) method that calls _provider.Dispose() (and Dispose() _crc32 if it implements IDisposable), set fields to null after disposing, and ensure Dispose is called by the owning context (or override the appropriate AutoContext cleanup hook) so IncrementalHash is released deterministically.
🧹 Nitpick comments (5)
src/OneScript.StandardLibrary/Hash/HashImpl.cs (3)
150-154: 💤 Low value
stream.Close()is redundant insideusing.The
usingblock already disposes theFileStream(which callsClose()), so line 153 is dead code.♻️ Suggested fix
using (var stream = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite)) { AppendStream(stream); - stream.Close(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/Hash/HashImpl.cs` around lines 150 - 154, Remove the redundant stream.Close() call inside the using block that opens the FileStream in HashImpl (the block that calls AppendStream(stream)); the using on new FileStream(...) already disposes/closes the stream, so delete the explicit stream.Close() to avoid dead code and rely on the using to handle disposal.
131-135: ⚡ Quick win
elsebranch is unbraced and unindented; add braces to avoid future bugs.The
elsebody on line 134 is not indented and has no braces, which obscures control flow and is a foot-gun if a second statement is ever added under the else.♻️ Suggested fix
case IStreamWrapper wrapper: var stream = wrapper.GetUnderlyingStream(); if (count <= 0) AppendStream(stream); else - AppendStream(stream, count); + AppendStream(stream, count); break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/Hash/HashImpl.cs` around lines 131 - 135, The if/else in HashImpl.cs around the AppendStream calls uses an unbraced, unindented else which risks incorrect control flow; update the conditional in the method containing the AppendStream calls so both branches use braces and proper indentation (e.g., change "if (count <= 0) AppendStream(stream); else AppendStream(stream, count);" to a braced form), ensuring the else branch around AppendStream(stream, count) is wrapped in { } and indented consistently.
74-90: ⚡ Quick winAvoid recomputing
IncrementalHash.GetCurrentHash()on every chunk.
AppendStreamreads in 32KB blocks and callsAppendDataeach time; for non-CRC32,AppendDatacurrently assigns_hash = _provider.GetCurrentHash()after every chunk. SinceIncrementalHash.GetCurrentHash()returns the hash for data accumulated so far without resetting the hasher, the code can defer materializing_hashuntilХешСумма/ХешСуммаСтрокойare requested (mark_hash“dirty” afterAppendData).♻️ Suggested approach
private void AppendData(byte[] data, int count) { if (_crc32 != null) { _crc32.AppendData(data,0,count); } else { _provider.AppendData(data,0,count); - _hash = _provider.GetCurrentHash(); + _hash = null; // mark dirty } }Then materialize lazily in
Hash(and apply the same lazy materialization inHashStringbefore reading_hash.Length):get { if (_crc32 != null) return ValueFactory.Create(_crc32.GetCurrentHashAsUInt32()); - return new BinaryDataContext(_hash); + _hash ??= _provider.GetCurrentHash(); + return new BinaryDataContext(_hash); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/Hash/HashImpl.cs` around lines 74 - 90, The AppendData(byte[] data, int count) path is recomputing _provider.GetCurrentHash() on every chunk; instead, stop materializing the hash there—when _crc32 is null simply call _provider.AppendData(data,0,count) and set _hash = null (mark dirty). Then lazily compute and cache _hash in the methods/properties that expose it (the ХешСумма and ХешСуммаСтрокой accessors) by calling _provider.GetCurrentHash() only when _hash is null and storing the result; ensure you also handle computing _hash.Length in ХешСуммаСтрокой after materialization. This applies to AppendData(byte[]), AppendData(byte[],int), and any code paths that read _hash.tests/datahashing-from-stream.os (1)
61-61: ⚡ Quick winRemove debug output left in the test.
Сообщить(Хеширование.ХешСуммаСтрокой);looks like a leftover from local debugging and only pollutes test output.🧹 Suggested fix
Хеширование.Добавить(ФайловыйПоток, 12); - Сообщить(Хеширование.ХешСуммаСтрокой); юТест.ПроверитьРавенство("3bc6abfa41bb73c8bcc4c0f5f4d4db42f10f38295a0432afc592fe998465cb9a"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/datahashing-from-stream.os` at line 61, Remove the leftover debug print by deleting the call Сообщить(Хеширование.ХешСуммаСтрокой); from the test; ensure no other direct reporting/logging calls remain in the test (look for Сообщить usages) so the test output is clean and only contains intended assertions.tests/hash.os (1)
115-176: ⚡ Quick winExtract the duplicated
ПустыеХешиmap into a helper.The same 6-entry empty-hash dictionary is repeated verbatim in
ТестДолжен_ПроверитьХешиБезДанных,ТестДолжен_ПроверитьХешиПустойСтроки, andТестДолжен_ПроверитьХешиПустогоПотока. Any future change (e.g., adding a newХешФункцияvalue) has to be replicated in three places. Extracting a small helper function reduces duplication and the risk of drift.♻️ Suggested fix
Функция ПолучитьХешиПустыхДанных() ПустыеХеши = Новый Соответствие(); ПустыеХеши.Вставить(ХешФункция.CRC32,"0"); ПустыеХеши.Вставить(ХешФункция.MD5,"D4 1D 8C D9 8F 00 B2 04 E9 80 09 98 EC F8 42 7E"); ПустыеХеши.Вставить(ХешФункция.SHA1,"DA 39 A3 EE 5E 6B 4B 0D 32 55 BF EF 95 60 18 90 AF D8 07 09"); ПустыеХеши.Вставить(ХешФункция.SHA256,"E3 B0 C4 42 98 FC 1C 14 9A FB F4 C8 99 6F B9 24 27 AE 41 E4 64 9B 93 4C A4 95 99 1B 78 52 B8 55"); ПустыеХеши.Вставить(ХешФункция.SHA384, "38 B0 60 A7 51 AC 96 38 4C D9 32 7E B1 B1 E3 6A 21 FD B7 11 14 BE 07 43 4C 0C C7 BF 63 F6 E1 DA 27 4E DE BF E7 6F 65 FB D5 1A D2 F1 48 98 B9 5B"); ПустыеХеши.Вставить(ХешФункция.SHA512, "CF 83 E1 35 7E EF B8 BD F1 54 28 50 D6 6D 80 07 D6 20 E4 05 0B 57 15 DC 83 F4 A9 21 D3 6C E9 CE 47 D0 D1 3C 5D 85 F2 B0 FF 83 18 D2 87 7E EC 2F 63 B9 31 BD 47 41 7A 81 A5 38 32 7A F9 27 DA 3E"); Возврат ПустыеХеши; КонецФункцииThen each test becomes
ПустыеХеши = ПолучитьХешиПустыхДанных();.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/hash.os` around lines 115 - 176, Дублирующийся словарь ПустыеХеши используется в ТестДолжен_ПроверитьХешиБезДанных, ТестДолжен_ПроверитьХешиПустойСтроки и ТестДолжен_ПроверитьХешиПустогоПотока — вынеси его в общую функцию ПолучитьХешиПустыхДанных(), которая создаёт и возвращает Соответствие с теми же 6 записями, а затем в трёх тестах замени локальные инициализации на вызов ПустыеХеши = ПолучитьХешиПустыхДанных(); чтобы убрать дублирование и упростить поддержку.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/OneScript.StandardLibrary/Hash/HashImpl.cs`:
- Around line 22-42: HashImpl currently holds an IncrementalHash in the
_provider field but no longer implements IDisposable, leaking native handles;
restore deterministic disposal by making HashImpl implement IDisposable (or the
script context lifecycle interface used by AutoContext), add a
Dispose/Dispose(bool) method that calls _provider.Dispose() (and Dispose()
_crc32 if it implements IDisposable), set fields to null after disposing, and
ensure Dispose is called by the owning context (or override the appropriate
AutoContext cleanup hook) so IncrementalHash is released deterministically.
---
Nitpick comments:
In `@src/OneScript.StandardLibrary/Hash/HashImpl.cs`:
- Around line 150-154: Remove the redundant stream.Close() call inside the using
block that opens the FileStream in HashImpl (the block that calls
AppendStream(stream)); the using on new FileStream(...) already disposes/closes
the stream, so delete the explicit stream.Close() to avoid dead code and rely on
the using to handle disposal.
- Around line 131-135: The if/else in HashImpl.cs around the AppendStream calls
uses an unbraced, unindented else which risks incorrect control flow; update the
conditional in the method containing the AppendStream calls so both branches use
braces and proper indentation (e.g., change "if (count <= 0)
AppendStream(stream); else AppendStream(stream, count);" to a braced form),
ensuring the else branch around AppendStream(stream, count) is wrapped in { }
and indented consistently.
- Around line 74-90: The AppendData(byte[] data, int count) path is recomputing
_provider.GetCurrentHash() on every chunk; instead, stop materializing the hash
there—when _crc32 is null simply call _provider.AppendData(data,0,count) and set
_hash = null (mark dirty). Then lazily compute and cache _hash in the
methods/properties that expose it (the ХешСумма and ХешСуммаСтрокой accessors)
by calling _provider.GetCurrentHash() only when _hash is null and storing the
result; ensure you also handle computing _hash.Length in ХешСуммаСтрокой after
materialization. This applies to AppendData(byte[]), AppendData(byte[],int), and
any code paths that read _hash.
In `@tests/datahashing-from-stream.os`:
- Line 61: Remove the leftover debug print by deleting the call
Сообщить(Хеширование.ХешСуммаСтрокой); from the test; ensure no other direct
reporting/logging calls remain in the test (look for Сообщить usages) so the
test output is clean and only contains intended assertions.
In `@tests/hash.os`:
- Around line 115-176: Дублирующийся словарь ПустыеХеши используется в
ТестДолжен_ПроверитьХешиБезДанных, ТестДолжен_ПроверитьХешиПустойСтроки и
ТестДолжен_ПроверитьХешиПустогоПотока — вынеси его в общую функцию
ПолучитьХешиПустыхДанных(), которая создаёт и возвращает Соответствие с теми же
6 записями, а затем в трёх тестах замени локальные инициализации на вызов
ПустыеХеши = ПолучитьХешиПустыхДанных(); чтобы убрать дублирование и упростить
поддержку.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d103adf5-6c95-44e8-a652-1d18eedd021a
📒 Files selected for processing (3)
src/OneScript.StandardLibrary/Hash/HashImpl.cstests/datahashing-from-stream.ostests/hash.os
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OneScript.StandardLibrary/Hash/HashImpl.cs (1)
145-148:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle direct
nullpaths explicitly.
File.Exists(null)just returnsfalse, so direct CLR callers end up withInvalidArgumentType()instead of a clear null-argument failure.🛡️ Proposed fix
public void AppendFile(string path) { + if (path == null) + throw new ArgumentNullException(nameof(path)); + if (!File.Exists(path)) throw RuntimeException.InvalidArgumentType();Based on learnings: "implement defensive null checks for these parameters in public methods that can be called from scripts."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/Hash/HashImpl.cs` around lines 145 - 148, The AppendFile method in HashImpl currently relies on File.Exists(path) which treats null as false; add an explicit null check at the start of public AppendFile(string path) and throw a clear null-argument exception (e.g., throw new ArgumentNullException(nameof(path))) before calling File.Exists so callers receive a proper null-argument failure instead of the generic InvalidArgumentType.
♻️ Duplicate comments (1)
src/OneScript.StandardLibrary/Hash/HashImpl.cs (1)
121-134:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject negative
countinstead of treating it as "read all".Line 131 still routes
count < 0through the full-stream branch, soДобавить(<stream>, -1)hashes the entire remaining stream instead of failing fast.🐛 Proposed fix
public void Append(BslValue toAdd, int count = 0) { + if (count < 0) + throw RuntimeException.InvalidArgumentValue(); + switch (toAdd) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/Hash/HashImpl.cs` around lines 121 - 134, The Append method currently treats count <= 0 as "read all", but the review asks to reject negative counts; update the Append(BslValue toAdd, int count = 0) implementation so that when toAdd is an IStreamWrapper you explicitly validate count: if count < 0 throw an appropriate exception (e.g., ArgumentOutOfRangeException) with a clear message, if count == 0 call AppendStream(stream) to read the whole stream, and if count > 0 call AppendStream(stream, count); keep the existing behavior for BslStringValue and continue using AppendData/AppendStream helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/OneScript.StandardLibrary/Hash/HashImpl.cs`:
- Around line 145-148: The AppendFile method in HashImpl currently relies on
File.Exists(path) which treats null as false; add an explicit null check at the
start of public AppendFile(string path) and throw a clear null-argument
exception (e.g., throw new ArgumentNullException(nameof(path))) before calling
File.Exists so callers receive a proper null-argument failure instead of the
generic InvalidArgumentType.
---
Duplicate comments:
In `@src/OneScript.StandardLibrary/Hash/HashImpl.cs`:
- Around line 121-134: The Append method currently treats count <= 0 as "read
all", but the review asks to reject negative counts; update the Append(BslValue
toAdd, int count = 0) implementation so that when toAdd is an IStreamWrapper you
explicitly validate count: if count < 0 throw an appropriate exception (e.g.,
ArgumentOutOfRangeException) with a clear message, if count == 0 call
AppendStream(stream) to read the whole stream, and if count > 0 call
AppendStream(stream, count); keep the existing behavior for BslStringValue and
continue using AppendData/AppendStream helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 616a2dc6-cf46-47ec-9982-a4fd992e19de
📒 Files selected for processing (2)
src/OneScript.StandardLibrary/Hash/HashImpl.cstests/datahashing-from-stream.os
💤 Files with no reviewable changes (1)
- tests/datahashing-from-stream.os
Mr-Rm
changed the title
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.
Summary by CodeRabbit
New Features
API Changes
Bug Fixes
Improvements
Tests