Refactor error handling: Replace Storage exceptions with error state management#47
Refactor error handling: Replace Storage exceptions with error state management#47
Conversation
…management Replace exception-based error handling with error state management for Storage stream errors. This change eliminates throwing and catching of: - Storage_StreamTypeMismatchError - Storage_StreamExtCharParityError - Storage_StreamWriteError - Storage_StreamFormatError - Storage_StreamReadError Changes: - Add error state management to Storage_BaseDriver with ErrorStatus(), ClearErrorStatus(), and SetErrorStatus() methods - Update FSD_File, FSD_BinaryFile, and FSD_CmpFile to set error state instead of throwing exceptions - Add error state management to BinTools_IStream - Update BinTools static methods to rely on stream error state - Replace try-catch blocks with error status checking - Mark all affected exception classes as deprecated The existing Storage_Error enum is used for error codes, maintaining compatibility with existing error handling patterns in OCCT.
…nd fix void function returns
…exception handling
|
Warning Rate limit exceeded@dpasukhi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughThe PR converts exception-based storage and I/O error handling to explicit status signaling. Try/catch blocks for Storage_Stream* exceptions were removed; drivers and file/stream classes now set and check a new Storage_BaseDriver error-state (ErrorStatus, ClearError, ErrorStatusExtension, protected SetErrorStatus). BinTools_IStream gained an internal error flag with HasError/ClearError. Several Storage_Stream* exception declarations were marked deprecated. Public APIs unchanged. Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
src/ApplicationFramework/TKStdL/StdLDrivers/StdLDrivers_DocumentRetrievalDriver.cxx (2)
24-26: Remove unused exception includes.These Storage exception includes are no longer needed after removing the try-catch blocks that handled these exceptions. The exceptions now propagate directly, so these headers are unused.
Apply this diff to remove the unused includes:
-#include <Storage_StreamTypeMismatchError.hxx> -#include <Storage_StreamFormatError.hxx> -#include <Storage_StreamReadError.hxx>
183-193: Remove dead error handling code.The
anErrorvariable is unconditionally set toStorage_VSOkafterReadReferenceTypesucceeds (line 186). IfReadReferenceTypethrows an exception, line 186 never executes and the exception propagates. This meansraiseOnStorageError(anError)at line 190 is always called withStorage_VSOk, making it effectively dead code that never raises an error.According to the PR objectives, error handling should use error state management. Either check
aFileDriver->ErrorStatus()after the call, or remove the redundant error checking entirely if exceptions are expected to propagate.Option 1: Use error state management (aligned with PR objectives)
Standard_Integer aRef = 0, aType = 0; - Storage_Error anError; { OCC_CATCH_SIGNALS aFileDriver->ReadReferenceType(aRef, aType); - anError = Storage_VSOk; } - - - raiseOnStorageError(anError); + raiseOnStorageError(aFileDriver->ErrorStatus());Option 2: Let exceptions propagate (simpler)
Standard_Integer aRef = 0, aType = 0; - Storage_Error anError; { OCC_CATCH_SIGNALS aFileDriver->ReadReferenceType(aRef, aType); - anError = Storage_VSOk; } - - - raiseOnStorageError(anError);src/ModelingData/TKBRep/BinTools/BinTools_IStream.cxx (1)
183-188: Critical: Inconsistent error handling in operator>>(Standard_Byte&) and operator>>(Standard_ShortReal&).These two overloads do NOT check the read result or set
myHasErroron failure, unlikeoperator>>(Standard_Real&)andoperator>>(Standard_Integer&)which do set the error flag. This inconsistency will lead to silent failures:
- A
Standard_ByteorStandard_ShortRealread failsmyHasErrorremainsStandard_False- Caller checks
HasError()and incorrectly assumes success- Corrupted data is used
Apply consistent error handling across all read operations:
BinTools_IStream& BinTools_IStream::operator>>(Standard_Byte& theValue) { - myStream->read((char*)&theValue, sizeof(Standard_Byte)); + if (!myStream->read((char*)&theValue, sizeof(Standard_Byte))) + myHasError = Standard_True; myPosition += sizeof(Standard_Byte); return *this; }BinTools_IStream& BinTools_IStream::operator>>(Standard_ShortReal& theValue) { - myStream->read((char*)&theValue, sizeof(Standard_ShortReal)); + if (!myStream->read((char*)&theValue, sizeof(Standard_ShortReal))) + myHasError = Standard_True; myPosition += sizeof(Standard_ShortReal); +#if DO_INVERSE + if (!myHasError) + theValue = InverseShortReal(theValue); +#endif return *this; }Also applies to: 194-199
src/FoundationClasses/TKernel/FSD/FSD_BinaryFile.cxx (2)
1302-1320: Incorrect error code used for read operation.Line 1312 sets
Storage_VSWriteErrorfor a read operation (ReadString). This should likely beStorage_VSReadErrororStorage_VSTypeMismatchto accurately reflect the nature of the error.Apply this diff to use the correct error code:
if (size > 0) { Standard_Character* c = (Standard_Character*)Standard::Allocate((size + 1) * sizeof(Standard_Character)); if (!fread(c, size, 1, myStream)) - SetErrorStatus(Storage_VSWriteError); + SetErrorStatus(Storage_VSReadError); c[size] = '\0'; aString = c; Standard::Free(c); }
1444-1466: Incorrect error code used for read operation.Line 1454 sets
Storage_VSWriteErrorfor a read operation (ReadExtendedString). This should beStorage_VSReadErrororStorage_VSTypeMismatchto accurately reflect the nature of the error.Apply this diff to use the correct error code:
if (size > 0) { Standard_ExtCharacter* c = (Standard_ExtCharacter*)Standard::Allocate((size + 1) * sizeof(Standard_ExtCharacter)); if (!fread(c, size * sizeof(Standard_ExtCharacter), 1, myStream)) - SetErrorStatus(Storage_VSWriteError); + SetErrorStatus(Storage_VSReadError); c[size] = '\0'; #if OCCT_BINARY_FILE_DO_INVERSE for (Standard_Integer i = 0; i < size; i++) c[i] = InverseExtChar(c[i]); #endif aString = c; Standard::Free(c); }src/FoundationClasses/TKernel/FSD/FSD_File.cxx (4)
1244-1362: Return immediately on format errors.With the new status-only flow, these
whileloops keep running afterSetErrorStatus(...). At EOF or on an unexpected delimiter,myStream.get(c)stops updatingc, so each loop spins forever and the caller never regains control. We must exit as soon as the format violation is detected.@@ - if (IsEnd() || (c != ' ') || (c == '\n')) - { - SetErrorStatus(Storage_VSFormatError); - } + if (IsEnd() || (c != ' ') || (c == '\n')) + { + SetErrorStatus(Storage_VSFormatError); + return; + } @@ - if (IsEnd() || (c != ' ') || (c == '\n')) - { - SetErrorStatus(Storage_VSFormatError); - } + if (IsEnd() || (c != ' ') || (c == '\n')) + { + SetErrorStatus(Storage_VSFormatError); + return; + } @@ - if (IsEnd() || (c != ' ') || (c == '\n')) - { - SetErrorStatus(Storage_VSFormatError); - } + if (IsEnd() || (c != ' ') || (c == '\n')) + { + SetErrorStatus(Storage_VSFormatError); + return; + } @@ - if (IsEnd() || (c != ' ') || (c == '\n')) - { - SetErrorStatus(Storage_VSFormatError); - } + if (IsEnd() || (c != ' ') || (c == '\n')) + { + SetErrorStatus(Storage_VSFormatError); + return; + } @@ - if (IsEnd() || (c != ' ') || (c == '\n')) - { - SetErrorStatus(Storage_VSFormatError); - } + if (IsEnd() || (c != ' ') || (c == '\n')) + { + SetErrorStatus(Storage_VSFormatError); + return; + } @@ - if (IsEnd() || (c != ' ') || (c == '\n')) - { - SetErrorStatus(Storage_VSFormatError); - } + if (IsEnd() || (c != ' ') || (c == '\n')) + { + SetErrorStatus(Storage_VSFormatError); + return; + } @@ - if (IsEnd() || (c != ' ')) - { - SetErrorStatus(Storage_VSFormatError); - } + if (IsEnd() || (c != ' ')) + { + SetErrorStatus(Storage_VSFormatError); + return; + }
765-806: Propagate write failures instead of returning success.All of these helpers now call
SetErrorStatus(Storage_VSWriteError)but still returnStorage_VSOk, so every caller treats a failed write as success. Please return the write error whenmyStream.bad()is set.@@ Storage_Error FSD_File::BeginWriteCommentSection() { myStream << "BEGIN_COMMENT_SECTION\n"; - if (myStream.bad()) - SetErrorStatus(Storage_VSWriteError); - return Storage_VSOk; + if (myStream.bad()) + { + SetErrorStatus(Storage_VSWriteError); + return Storage_VSWriteError; + } + return Storage_VSOk; } @@ Storage_Error FSD_File::EndWriteCommentSection() { myStream << "END_COMMENT_SECTION\n"; - if (myStream.bad()) - SetErrorStatus(Storage_VSWriteError); - return Storage_VSOk; + if (myStream.bad()) + { + SetErrorStatus(Storage_VSWriteError); + return Storage_VSWriteError; + } + return Storage_VSOk; } @@ Storage_Error FSD_File::BeginWriteTypeSection() { myStream << "BEGIN_TYPE_SECTION\n"; - if (myStream.bad()) - SetErrorStatus(Storage_VSWriteError); - return Storage_VSOk; + if (myStream.bad()) + { + SetErrorStatus(Storage_VSWriteError); + return Storage_VSWriteError; + } + return Storage_VSOk; } @@ Storage_Error FSD_File::EndWriteTypeSection() { myStream << "END_TYPE_SECTION\n"; - if (myStream.bad()) - SetErrorStatus(Storage_VSWriteError); - return Storage_VSOk; + if (myStream.bad()) + { + SetErrorStatus(Storage_VSWriteError); + return Storage_VSWriteError; + } + return Storage_VSOk; } @@ Storage_Error FSD_File::BeginWriteRootSection() { myStream << "BEGIN_ROOT_SECTION\n"; - if (myStream.bad()) - SetErrorStatus(Storage_VSWriteError); - return Storage_VSOk; + if (myStream.bad()) + { + SetErrorStatus(Storage_VSWriteError); + return Storage_VSWriteError; + } + return Storage_VSOk; } @@ Storage_Error FSD_File::EndWriteRootSection() { myStream << "END_ROOT_SECTION\n"; - if (myStream.bad()) - SetErrorStatus(Storage_VSWriteError); - return Storage_VSOk; + if (myStream.bad()) + { + SetErrorStatus(Storage_VSWriteError); + return Storage_VSWriteError; + } + return Storage_VSOk; } @@ Storage_Error FSD_File::BeginWriteRefSection() { myStream << "BEGIN_REF_SECTION\n"; - if (myStream.bad()) - SetErrorStatus(Storage_VSWriteError); - return Storage_VSOk; + if (myStream.bad()) + { + SetErrorStatus(Storage_VSWriteError); + return Storage_VSWriteError; + } + return Storage_VSOk; } @@ Storage_Error FSD_File::EndWriteRefSection() { myStream << "END_REF_SECTION\n"; - if (myStream.bad()) - SetErrorStatus(Storage_VSWriteError); - return Storage_VSOk; + if (myStream.bad()) + { + SetErrorStatus(Storage_VSWriteError); + return Storage_VSWriteError; + } + return Storage_VSOk; } @@ Storage_Error FSD_File::BeginWriteDataSection() { myStream << "BEGIN_DATA_SECTION"; - if (myStream.bad()) - SetErrorStatus(Storage_VSWriteError); - return Storage_VSOk; + if (myStream.bad()) + { + SetErrorStatus(Storage_VSWriteError); + return Storage_VSWriteError; + } + return Storage_VSOk; } @@ Storage_Error FSD_File::EndWriteDataSection() { myStream << "\nEND_DATA_SECTION\n"; - if (myStream.bad()) - SetErrorStatus(Storage_VSWriteError); - return Storage_VSOk; + if (myStream.bad()) + { + SetErrorStatus(Storage_VSWriteError); + return Storage_VSWriteError; + } + return Storage_VSOk; }Also applies to: 850-891, 950-1090, 1147-1224
825-835: Stop when reading the comment length fails.If the length cannot be extracted we now set the error status but continue.
lenremains indeterminate, so the subsequent loop iterates an arbitrary number of times and corrupts state. Return immediately after flagging the mismatch.if (!(myStream >> len)) - SetErrorStatus(Storage_VSTypeMismatch); + { + SetErrorStatus(Storage_VSTypeMismatch); + return; + }
905-915: Return a safe default when section size parsing fails.
TypeSectionSize(),RootSectionSize(), andRefSectionSize()now set the error flag but still returni, which is uninitialised when extraction fails. Mirror the old exception semantics by bailing out with a defined value (e.g.0) as soon as the read breaks.Standard_Integer FSD_File::TypeSectionSize() { - Standard_Integer i; - - if (!(myStream >> i)) - SetErrorStatus(Storage_VSTypeMismatch); + Standard_Integer i = 0; + + if (!(myStream >> i)) + { + SetErrorStatus(Storage_VSTypeMismatch); + return 0; + } @@ Standard_Integer FSD_File::RootSectionSize() { - Standard_Integer i; - - if (!(myStream >> i)) - SetErrorStatus(Storage_VSTypeMismatch); + Standard_Integer i = 0; + + if (!(myStream >> i)) + { + SetErrorStatus(Storage_VSTypeMismatch); + return 0; + } @@ Standard_Integer FSD_File::RefSectionSize() { - Standard_Integer i; - - if (!(myStream >> i)) - SetErrorStatus(Storage_VSTypeMismatch); + Standard_Integer i = 0; + + if (!(myStream >> i)) + { + SetErrorStatus(Storage_VSTypeMismatch); + return 0; + }Also applies to: 1006-1015, 1103-1112
src/FoundationClasses/TKernel/Storage/Storage_RootData.cxx (1)
52-63: Check the driver’s error status before binding roots.After
ReadRootreturns, the driver may have setErrorStatus()to report a type mismatch or format error. We now ignore that and still bind a root, leavingStorage_RootDatain an inconsistent state. Interrogate the driver status and abort the loop when it is notStorage_VSOk.{ OCC_CATCH_SIGNALS theDriver->ReadRoot(aRootName, aRef, aTypeName); } - + if (theDriver->ErrorStatus() != Storage_VSOk) + { + myErrorStatus = theDriver->ErrorStatus(); + myErrorStatusExt = theDriver->ErrorStatusExtension(); + return Standard_False; + }src/ApplicationFramework/TKStd/StdStorage/StdStorage.cxx (1)
124-138: Preserve errors raised by the driver.
ReadReferenceTypeandReadPersistentObjectnow rely on the driver’s error status. OverwritinganErrorwithStorage_VSOkhides those failures, so the read continues with corrupt state. Capture the driver’sErrorStatus()right after each call (and fall back immediately if it is notStorage_VSOk).{ OCC_CATCH_SIGNALS theDriver->ReadReferenceType(aRef, aType); - anError = Storage_VSOk; } - + anError = theDriver->ErrorStatus(); if (anError != Storage_VSOk) return anError; @@ { OCC_CATCH_SIGNALS aReadData.ReadPersistentObject(i); - anError = Storage_VSOk; } - - - + anError = theDriver->ErrorStatus(); if (anError != Storage_VSOk) return anError;Also applies to: 149-162
src/ApplicationFramework/TKStd/StdStorage/StdStorage_HeaderData.cxx (1)
48-90: Propagate driver errors after info/comment I/O.When
ReadInfo,ReadComment,WriteInfo, orWriteCommentfails, the driver sets its error status. We currently ignore that and continue, so callers believe everything succeeded. Capture the driver’s status (and extension) right after each call and returnStandard_Falsewhen it is notStorage_VSOk.{ OCC_CATCH_SIGNALS theDriver->ReadInfo(myNBObj, myStorageVersion, myDate, mySchemaName, mySchemaVersion, myApplicationName, myApplicationVersion, myDataType, myUserInfo); } - - + if (theDriver->ErrorStatus() != Storage_VSOk) + { + myErrorStatus = theDriver->ErrorStatus(); + myErrorStatusExt = theDriver->ErrorStatusExtension(); + return Standard_False; + } @@ { OCC_CATCH_SIGNALS theDriver->ReadComment(myComments); } - - + if (theDriver->ErrorStatus() != Storage_VSOk) + { + myErrorStatus = theDriver->ErrorStatus(); + myErrorStatusExt = theDriver->ErrorStatusExtension(); + return Standard_False; + } @@ { OCC_CATCH_SIGNALS theDriver->WriteInfo(myNBObj, myStorageVersion, myDate, mySchemaName, mySchemaVersion, myApplicationName, myApplicationVersion, myDataType, myUserInfo); } - - + if (theDriver->ErrorStatus() != Storage_VSOk) + { + myErrorStatus = theDriver->ErrorStatus(); + myErrorStatusExt = theDriver->ErrorStatusExtension(); + return Standard_False; + } @@ { OCC_CATCH_SIGNALS theDriver->WriteComment(myComments); } - - + if (theDriver->ErrorStatus() != Storage_VSOk) + { + myErrorStatus = theDriver->ErrorStatus(); + myErrorStatusExt = theDriver->ErrorStatusExtension(); + return Standard_False; + }Also applies to: 113-155
src/ApplicationFramework/TKStd/StdStorage/StdStorage_RootData.cxx (1)
50-61: Critical: Missing error status check after ReadRoot.The code removes exception handling but doesn't check the driver's error status after
ReadRoot. IfReadRootfails for any iteration (e.g., item 3 of 10), the loop continues processing remaining items, potentially creating roots with corrupted data.Apply this diff to add error status checking consistent with the pattern in
Storage_HeaderData.cxx:for (Standard_Integer i = 1; i <= len; i++) { { OCC_CATCH_SIGNALS theDriver->ReadRoot(aRootName, aRef, aTypeName); + + // Check for errors from the driver + if (theDriver->ErrorStatus() != Storage_VSOk) + { + myErrorStatus = theDriver->ErrorStatus(); + myErrorStatusExt = "ReadRoot"; + return Standard_False; + } } - Handle(StdStorage_Root) aRoot = new StdStorage_Root(aRootName, aRef, aTypeName); myObjects.Add(aRootName, aRoot);
🧹 Nitpick comments (7)
src/ModelingData/TKBRep/BinTools/BinTools_IStream.hxx (1)
62-66: Consider adding documentation for the error state management API.The new error flag methods provide a basic error state mechanism, but there's no documentation about:
- When callers should check
HasError()- When callers should call
ClearError()- What error occurred (the flag is binary, providing no error details)
Since the PR mentions using
Storage_VS*error codes elsewhere, consider whetherBinTools_IStreamshould also track a specific error code rather than just a boolean flag.Add documentation to clarify usage:
+ //! Returns true if an error occurred during the last read operation. + //! Callers should check this after reading operations to detect failures. Standard_Boolean HasError() const { return myHasError; } + //! Clears the error status. Should be called before starting a new sequence of read operations. void ClearError() { myHasError = Standard_False; }src/ModelingData/TKBRep/BinTools/BinTools_IStream.cxx (2)
133-142: Error handling is good, but note DO_INVERSE behavior differs from BinTools.cxx.The error detection correctly sets
myHasError = Standard_Trueon read failure. However, there's a subtle difference fromBinTools.cxx:
BinTools.cxxonly inverts if the stream is good (lines 87-88)- This code always inverts, even on read failure
While inverting garbage data is harmless, the inconsistency is worth noting.
Consider aligning with the pattern in
BinTools.cxx:BinTools_IStream& BinTools_IStream::operator>>(Standard_Real& theValue) { if (!myStream->read((char*)&theValue, sizeof(Standard_Real))) myHasError = Standard_True; myPosition += sizeof(Standard_Real); #if DO_INVERSE - theValue = InverseReal(theValue); + if (!myHasError) + theValue = InverseReal(theValue); #endif return *this; }
163-177: Verify partial read behavior is intentional for gp_Pnt.The code sets
myHasErroron any coordinate read failure but continues reading the remaining coordinates. This means:
- If the first coordinate read fails, the remaining two are still attempted
- The point will contain a mix of garbage and valid data
- Stream position is consistently advanced
This behavior is likely intentional to maintain stream position consistency, but it should be documented.
Is this the intended behavior? Should the loop break early on the first read failure?
If early exit is preferred:
BinTools_IStream& BinTools_IStream::operator>>(gp_Pnt& theValue) { Standard_Real aValue; for (int aCoord = 1; aCoord <= 3; aCoord++) { if (!myStream->read((char*)&aValue, sizeof(Standard_Real))) + { myHasError = Standard_True; + break; + } #if DO_INVERSE aValue = InverseReal(aValue); #endif theValue.SetCoord(aCoord, aValue); } myPosition += 3 * sizeof(Standard_Real); return *this; }src/FoundationClasses/TKernel/FSD/FSD_BinaryFile.cxx (4)
333-340: Consider setting error status in IStream variant of GetReference.This IStream-based variant only conditionally inverts the value if the read succeeded (checking
gcount), but it doesn't explicitly set error status when the read fails. This is inconsistent with the FILE*-based variant at lines 319-326 which callsSetErrorStatus(Storage_VSTypeMismatch)on failure.For consistency and proper error propagation, consider adding error status handling similar to other Get methods.
Apply this diff to add error status handling:
void FSD_BinaryFile::GetReference(Standard_IStream& theIStream, Standard_Integer& aValue) { theIStream.read((char*)&aValue, sizeof(Standard_Integer)); #if OCCT_BINARY_FILE_DO_INVERSE if (theIStream.gcount() == sizeof(Standard_Integer)) aValue = InverseInt(aValue); + else + SetErrorStatus(Storage_VSTypeMismatch); #endif }
378-386: Consider setting error status in IStream variant of GetInteger.Similar to
GetReferenceabove, this IStream variant doesn't set error status on read failure. For consistency with the FILE*-based variant and other Get methods, consider adding error status handling.Apply this diff to add error status handling:
void FSD_BinaryFile::GetInteger(Standard_IStream& theIStream, Standard_Integer& theValue) { theIStream.read((char*)&theValue, sizeof(Standard_Integer)); #if OCCT_BINARY_FILE_DO_INVERSE if (theIStream.gcount() == sizeof(Standard_Integer)) theValue = InverseInt(theValue); + else + SetErrorStatus(Storage_VSTypeMismatch); #endif }
1327-1363: Inconsistent error handling in IStream variant of ReadString.This IStream-based variant clears the output string and returns early on read failures (lines 1340-1342, 1349-1351), but it doesn't call
SetErrorStatus. This is inconsistent with:
- The FILE*-based
ReadStringvariant which sets error status- Other IStream Get methods pattern
For consistency, consider setting error status before the early returns.
Apply this diff to add error status handling:
if (!theIStream.good()) { + SetErrorStatus(Storage_VSReadError); aString.Clear(); Standard::Free(c); return; } theIStream.read(c, size); if (theIStream.gcount() != size) { + SetErrorStatus(Storage_VSReadError); aString.Clear(); Standard::Free(c); return; }
1473-1515: Inconsistent error handling in IStream variant of ReadExtendedString.Similar to
ReadString, this IStream variant clears output and returns early on failures (lines 1487-1489, 1496-1498) without callingSetErrorStatus. For consistency with other error handling patterns in this refactoring, consider setting error status.Apply this diff to add error status handling:
if (!theIStream.good()) { + SetErrorStatus(Storage_VSReadError); aString.Clear(); Standard::Free(c); return; } const std::streamsize aNbBytes = std::streamsize(sizeof(Standard_ExtCharacter) * size); theIStream.read((char*)c, aNbBytes); if (theIStream.gcount() != aNbBytes) { + SetErrorStatus(Storage_VSReadError); aString.Clear(); Standard::Free(c); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/ApplicationFramework/TKStd/StdStorage/StdStorage.cxx(3 hunks)src/ApplicationFramework/TKStd/StdStorage/StdStorage_HeaderData.cxx(4 hunks)src/ApplicationFramework/TKStd/StdStorage/StdStorage_RootData.cxx(2 hunks)src/ApplicationFramework/TKStd/StdStorage/StdStorage_TypeData.cxx(2 hunks)src/ApplicationFramework/TKStdL/StdLDrivers/StdLDrivers_DocumentRetrievalDriver.cxx(2 hunks)src/FoundationClasses/TKernel/FSD/FSD_BinaryFile.cxx(23 hunks)src/FoundationClasses/TKernel/FSD/FSD_CmpFile.cxx(4 hunks)src/FoundationClasses/TKernel/FSD/FSD_File.cxx(56 hunks)src/FoundationClasses/TKernel/Storage/Storage_BaseDriver.cxx(1 hunks)src/FoundationClasses/TKernel/Storage/Storage_BaseDriver.hxx(2 hunks)src/FoundationClasses/TKernel/Storage/Storage_HeaderData.cxx(2 hunks)src/FoundationClasses/TKernel/Storage/Storage_RootData.cxx(1 hunks)src/FoundationClasses/TKernel/Storage/Storage_StreamExtCharParityError.hxx(2 hunks)src/FoundationClasses/TKernel/Storage/Storage_StreamFormatError.hxx(2 hunks)src/FoundationClasses/TKernel/Storage/Storage_StreamReadError.hxx(2 hunks)src/FoundationClasses/TKernel/Storage/Storage_StreamTypeMismatchError.hxx(2 hunks)src/FoundationClasses/TKernel/Storage/Storage_StreamWriteError.hxx(2 hunks)src/ModelingData/TKBRep/BinTools/BinTools.cxx(4 hunks)src/ModelingData/TKBRep/BinTools/BinTools_IStream.cxx(4 hunks)src/ModelingData/TKBRep/BinTools/BinTools_IStream.hxx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/ModelingData/TKBRep/BinTools/BinTools.cxx (2)
src/ModelingData/TKBRep/BinTools/BinTools_IStream.cxx (12)
theValue(133-142)theValue(133-133)theValue(148-157)theValue(148-148)theValue(163-177)theValue(163-163)theValue(183-188)theValue(183-183)theValue(194-199)theValue(194-194)theValue(205-224)theValue(205-205)src/FoundationClasses/TKernel/FSD/FSD_BinaryFile.cxx (4)
InverseReal(1662-1678)InverseReal(1662-1662)InverseShortReal(1685-1698)InverseShortReal(1685-1685)
src/FoundationClasses/TKernel/FSD/FSD_File.cxx (7)
src/ApplicationFramework/TKStd/StdStorage/StdStorage_HeaderData.cxx (2)
SetErrorStatus(260-263)SetErrorStatus(260-260)src/ApplicationFramework/TKStd/StdStorage/StdStorage_RootData.cxx (2)
SetErrorStatus(177-180)SetErrorStatus(177-177)src/ApplicationFramework/TKStd/StdStorage/StdStorage_TypeData.cxx (2)
SetErrorStatus(213-216)SetErrorStatus(213-213)src/FoundationClasses/TKernel/Storage/Storage_HeaderData.cxx (2)
SetErrorStatus(213-216)SetErrorStatus(213-213)src/FoundationClasses/TKernel/Storage/Storage_RootData.cxx (2)
SetErrorStatus(141-144)SetErrorStatus(141-141)src/FoundationClasses/TKernel/FSD/FSD_CmpFile.cxx (2)
WriteExtendedLine(157-185)WriteExtendedLine(157-157)src/FoundationClasses/TKernel/FSD/FSD_BinaryFile.cxx (2)
IsEnd(130-133)IsEnd(130-130)
src/FoundationClasses/TKernel/FSD/FSD_BinaryFile.cxx (2)
src/FoundationClasses/TKernel/Storage/Storage_HeaderData.cxx (2)
SetErrorStatus(213-216)SetErrorStatus(213-213)src/ModelingData/TKBRep/BinTools/BinTools_IStream.cxx (13)
myStream(124-127)theValue(133-142)theValue(133-133)theValue(148-157)theValue(148-148)theValue(163-177)theValue(163-163)theValue(183-188)theValue(183-183)theValue(194-199)theValue(194-194)theValue(205-224)theValue(205-205)
src/FoundationClasses/TKernel/FSD/FSD_CmpFile.cxx (2)
src/FoundationClasses/TKernel/Storage/Storage_HeaderData.cxx (2)
SetErrorStatus(213-216)SetErrorStatus(213-213)src/FoundationClasses/TKernel/FSD/FSD_File.cxx (10)
BeginWritePersistentObjectData(1170-1178)BeginWritePersistentObjectData(1170-1170)BeginWriteObjectData(1182-1190)BeginWriteObjectData(1182-1182)EndWriteObjectData(1194-1202)EndWriteObjectData(1194-1194)EndWritePersistentObjectData(1206-1214)EndWritePersistentObjectData(1206-1206)IsEnd(125-128)IsEnd(125-125)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Prepare and Build on macOS with Clang (No PCH)
- GitHub Check: Prepare and Build on Ubuntu with Clang (x64)
- GitHub Check: Prepare and Build on macOS with Clang (x64)
- GitHub Check: Prepare and Build on Windows with MSVC (x64)
- GitHub Check: Build Documentation
🔇 Additional comments (22)
src/ModelingData/TKBRep/BinTools/BinTools_IStream.cxx (1)
19-25: LGTM! Proper initialization of the error flag.The
myHasErrormember is correctly initialized toStandard_Falsein the member initializer list, following C++ best practices.src/FoundationClasses/TKernel/Storage/Storage_BaseDriver.cxx (1)
22-25: LGTM! Clean initialization of error state.The constructor properly initializes the new
myErrorStatusmember toStorage_VSOk, establishing a clear "no error" initial state that aligns with the error state management approach.src/ApplicationFramework/TKStd/StdStorage/StdStorage_TypeData.cxx (1)
95-99: Verify exception propagation behavior after removing per-item exception handling.Similar to the Read method, the per-item try-catch block has been removed here. Ensure that
WriteTypeInformationsnow signals errors via status codes rather than exceptions.src/FoundationClasses/TKernel/Storage/Storage_StreamExtCharParityError.hxx (1)
25-39: LGTM! Clear deprecation documentation.The deprecation comments appropriately document that this exception class is being replaced by error state management using the
Storage_Errorenum. This provides clear guidance for users of the API.src/FoundationClasses/TKernel/Storage/Storage_StreamWriteError.hxx (1)
25-39: LGTM! Clear deprecation documentation.The deprecation comments appropriately document that this exception class is being replaced by error state management.
src/FoundationClasses/TKernel/Storage/Storage_StreamTypeMismatchError.hxx (1)
25-39: LGTM! Clear deprecation documentation.The deprecation comments appropriately document that this exception class is being replaced by error state management.
src/FoundationClasses/TKernel/Storage/Storage_StreamReadError.hxx (1)
25-39: LGTM! Clear deprecation documentation.The deprecation comments appropriately document that this exception class is being replaced by error state management.
src/FoundationClasses/TKernel/FSD/FSD_CmpFile.cxx (4)
239-246: LGTM! Clean error status return.The method now returns
Storage_VSWriteErrordirectly instead of throwing an exception, which aligns with the status-based error handling approach.
273-326: LGTM! Consistent error status handling across write methods.All write methods (
WritePersistentObjectHeader,BeginWritePersistentObjectData,BeginWriteObjectData,EndWriteObjectData,EndWritePersistentObjectData) now consistently useSetErrorStatus(Storage_VSWriteError)followed by early return instead of throwing exceptions.Ensure that callers of these void methods check
ErrorStatus()after calling them to detect write failures.
330-369: LGTM! Proper error status handling for read format errors.The method correctly sets appropriate error statuses:
Storage_VSFormatErrorfor format issues (invalid separators, unexpected characters)Storage_VSTypeMismatchfor type reading failuresThe early returns prevent further processing when errors are detected.
395-413: LGTM! Correct error status handling for trailing data validation.The method properly validates trailing characters and sets
Storage_VSFormatErrorwhen non-whitespace characters are encountered, followed by an early return.src/FoundationClasses/TKernel/FSD/FSD_BinaryFile.cxx (7)
192-243: LGTM! Consistent error status handling for Put methods.All Put methods (
PutReference,PutCharacter,PutExtCharacter,PutInteger,PutBoolean) now setStorage_VSWriteErrorstatus on write failures instead of throwing exceptions. The pattern is applied consistently across all these methods.
270-311: LGTM! Continued consistent error status handling.The remaining Put methods (
PutBoolean,PutReal,PutShortReal) follow the same consistent pattern of settingStorage_VSWriteErroron failures.
319-326: LGTM! Error status handling for GetReference.The method correctly sets
Storage_VSTypeMismatchwhen the read operation fails.
345-424: LGTM! Consistent error status handling for Get methods.All Get methods for FILE* operations (
GetCharacter,GetExtCharacter,GetInteger,GetBoolean,GetReal,GetShortReal) consistently setStorage_VSTypeMismatchon read failures.
442-462: LGTM! Error status handling in BeginWriteInfoSection.The method correctly uses
SetErrorStatus(Storage_VSWriteError)when writing the magic number fails, replacing the previous exception-based approach.
1260-1272: LGTM! Error status handling in WriteString.The method correctly sets
Storage_VSWriteErrorwhen the write operation fails.
1371-1397: LGTM! Error status handling in WriteExtendedString.The method correctly sets
Storage_VSWriteErrorwhen the write operation fails.src/FoundationClasses/TKernel/Storage/Storage_HeaderData.cxx (2)
85-96: LGTM! Consistent error handling pattern.The
ReadCommentsection follows the same correct pattern asReadInfo: wrap inOCC_CATCH_SIGNALS, check driver error status, and propagate errors appropriately.
49-68: LGTM. OCC_CATCH_SIGNALS coverage is appropriate.The signal-based exception handling pattern is correctly implemented and consistent with the codebase design:
OCC_CATCH_SIGNALSconverts OS-level signals (SIGSEGV, division by zero, etc.) to C++ exceptions whenOCC_CONVERT_SIGNALSis enabled, then reraises them. This ensures unrecoverable signal conditions abort safely rather than silently corrupt state.- Status-based errors are handled immediately after via
theDriver->ErrorStatus()checks, enabling normal error propagation.- The pattern matches usage in related code (Storage_RootData, Storage_TypeData, etc.).
src/FoundationClasses/TKernel/Storage/Storage_BaseDriver.hxx (2)
196-214: LGTM! Well-designed error state management API.The API design follows good practices:
- Public getters for querying error state
- Public
ClearErrorStatus()for resetting state- Protected
SetErrorStatus()restricts error setting to derived classesErrorStatusExtension()returnsconst&to avoid unnecessary copies- Default empty string parameter for
SetErrorStatus()provides convenience- Inline implementation for efficiency
288-289: LGTM! Appropriate private member placement.The private members
myErrorStatusandmyErrorStatusExtare correctly placed and encapsulated with the public/protected accessors defined above.
src/ApplicationFramework/TKStd/StdStorage/StdStorage_TypeData.cxx
Outdated
Show resolved
Hide resolved
src/ApplicationFramework/TKStdL/StdLDrivers/StdLDrivers_DocumentRetrievalDriver.cxx
Outdated
Show resolved
Hide resolved
| Standard_IStream& BinTools::GetReal(Standard_IStream& theIS, Standard_Real& theValue) | ||
| { | ||
| if (!theIS.read((char*)&theValue, sizeof(Standard_Real))) | ||
| { | ||
| throw Storage_StreamTypeMismatchError(); | ||
| } | ||
| theIS.read((char*)&theValue, sizeof(Standard_Real)); | ||
| #ifdef DO_INVERSE | ||
| theValue = InverseReal(theValue); | ||
| if (theIS.good()) | ||
| theValue = InverseReal(theValue); | ||
| #endif | ||
| return theIS; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Potential data corruption risk: verify callers check stream state.
The refactored code reads directly without checking success, then conditionally applies inversion only if the stream is good. If the read fails:
theValuecontains uninitialized or partial data- No inversion is applied (correct for error case)
- The caller receives potentially corrupted data
This pattern shifts error detection responsibility to the caller, who must now check the stream state after calling these methods. If any caller fails to check the stream state, it will silently use corrupted data.
Verify that all callers of BinTools::GetReal check the stream state after calling:
🏁 Script executed:
#!/bin/bash
# Find all calls to BinTools::GetReal and check if they verify stream state
rg -n -C5 'BinTools::GetReal' --type=cppLength of output: 54988
All callers fail to check stream state—data corruption risk is confirmed.
Verification found no stream state checks in any of the 60+ call sites across BinTools_SurfaceSet.cxx, BinTools_ShapeSet.cxx, BinTools_CurveSet.cxx, BinTools_Curve2dSet.cxx, and BinTools_LocationSet.cxx. Callers chain BinTools::GetReal calls sequentially and immediately use the values without verifying stream success. For example:
BinTools::GetReal(IS, X);
BinTools::GetReal(IS, Y);
BinTools::GetReal(IS, Z);
P.SetCoord(X, Y, Z); // Uses data even if reads failed
return IS;
If any read fails, variables retain initial values (0 or uninitialized) and are silently used in object construction. Requires stream state verification at each call site.
| Standard_IStream& BinTools::GetShortReal(Standard_IStream& theIS, Standard_ShortReal& theValue) | ||
| { | ||
| if (!theIS.read((char*)&theValue, sizeof(Standard_ShortReal))) | ||
| { | ||
| throw Storage_StreamTypeMismatchError(); | ||
| } | ||
| theIS.read((char*)&theValue, sizeof(Standard_ShortReal)); | ||
| #ifdef DO_INVERSE | ||
| theValue = InverseShortReal(theValue); | ||
| if (theIS.good()) | ||
| theValue = InverseShortReal(theValue); | ||
| #endif | ||
| return theIS; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Same data corruption risk affects GetShortReal, GetInteger, and GetExtChar.
These methods follow the same pattern as GetReal (lines 83-91): they read unconditionally and only apply inversion if the stream is good. This creates the same risk of silent data corruption if callers don't check the stream state.
Verify that all callers of these methods check stream state:
Also applies to: 107-115, 119-127
🏁 Script executed:
#!/bin/bash
# Find all calls to these BinTools Get* methods
rg -n -C5 'BinTools::(GetShortReal|GetInteger|GetExtChar)' --type=cppLength of output: 36500
Wait - need to search in repository:
</parameter>
</invoke>
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if callers check stream state after Get* calls
# Look for patterns where IS.good(), IS.fail(), or explicit checks follow Get* method calls
rg -n -A2 'BinTools::(GetShortReal|GetInteger|GetExtChar)' --type=cpp | \
grep -A2 'BinTools::Get' | \
head -100
Length of output: 7987
🏁 Script executed:
#!/bin/bash
# Look for stream state checks after Get* method calls in the same files
# Check if there's a consistent pattern of error checking
rg -n 'if.*\.(good|fail|rdstate|operator!)' --type=cpp src/ModelingData/TKBRep/BinTools/ | head -30Length of output: 1258
🏁 Script executed:
#!/bin/bash
# Check a representative function - ReadSurface in BinTools_SurfaceSet.cxx
# Focus on lines around 604-611 where consecutive GetInteger calls happen
sed -n '550,670p' src/ModelingData/TKBRep/BinTools/BinTools_SurfaceSet.cxxLength of output: 4043
Data corruption risk confirmed in GetShortReal, GetInteger, and GetExtChar methods.
Verification shows these methods are called consecutively without stream state validation between calls. In BinTools_SurfaceSet.cxx lines 604-611, four GetInteger calls populate array dimensions, then arrays are constructed immediately without checking if any read failed. If a read fails partway through, subsequent values remain uninitialized/garbage, leading to corrupted array bounds and silent data loss.
The internal if (theIS.good()) checks in these methods guard only the inversion operation, not read success. They do not prevent corruption when callers chain multiple Get* calls.
Affected call sites (confirmed from search):
- Lines 558-562, 604-611, 632, 640 in BinTools_SurfaceSet.cxx (consecutive GetInteger without checks)
- Lines 838, 847-848, 936-937, 1556-1558 in BinTools_ShapeSet.cxx
- Lines 429, 463, 466, 468, 486 in BinTools_CurveSet.cxx
- Lines 419, 452, 455, 457, 475 in BinTools_Curve2dSet.cxx
Callers must validate stream state after each Get* sequence or add stream error checks between consecutive reads.
🤖 Prompt for AI Agents
In src/ModelingData/TKBRep/BinTools/BinTools.cxx around lines 95-103, the method
reads bytes into theValue unconditionally so a partial or failed read can leave
theValue with garbage; move the read-success check to guard assignment so
theValue is only modified when the read actually succeeded and set/propagate the
stream fail state on error. Concretely: perform theIS.read(...) then test
theIS.good()/!theIS.fail() before touching or inverting theValue; ensure the
method returns the stream with the appropriate failbit if read failed, and
update callers (or document) to check the stream state between consecutive Get*
calls so they do not use uninitialized values after a failed read.
…ssues This commit addresses multiple critical and major issues identified by Rabbit AI code review: 1. Fixed dead error handling code in StdLDrivers_DocumentRetrievalDriver.cxx: - Lines 182-190: Removed unconditional Storage_VSOk assignment after ReadReferenceType - Lines 202-212: Removed unconditional Storage_VSOk assignment after ReadPersistentObject - Both now properly check driver->ErrorStatus() to detect failures 2. Fixed incorrect error codes in FSD_BinaryFile.cxx: - ReadString (line 1312): Changed Storage_VSWriteError to Storage_VSTypeMismatch for fread failure - ReadExtendedString (line 1454): Changed Storage_VSWriteError to Storage_VSTypeMismatch for fread failure - Now consistent with other read operations in the same file 3. Added missing error checks in StdStorage_RootData.cxx: - Added driver error status check after ReadRoot call (line 58-63) - Added driver error status check after WriteRoot call (line 106-111) - Prevents silent failures when root section read/write operations fail 4. Enhanced error handling documentation in BinTools_IStream.hxx: - Added detailed comments for HasError() method explaining when to check for errors - Added detailed comments for ClearError() method explaining when to reset error state - Clarifies error handling contract for stream read operations These fixes address data corruption risks and ensure errors are properly propagated to callers.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/FoundationClasses/TKernel/FSD/FSD_BinaryFile.cxx (1)
1453-1461: Stop building extended strings after failed freadThe extended-string path has the same problem as the ASCII one: on a short read we still reuse the scratch buffer. Bail out immediately after setting the status and release the allocation.
Standard_ExtCharacter* c = (Standard_ExtCharacter*)Standard::Allocate((size + 1) * sizeof(Standard_ExtCharacter)); - if (!fread(c, size * sizeof(Standard_ExtCharacter), 1, myStream)) - SetErrorStatus(Storage_VSTypeMismatch); + if (!fread(c, size * sizeof(Standard_ExtCharacter), 1, myStream)) + { + SetErrorStatus(Storage_VSTypeMismatch); + aString.Clear(); + Standard::Free(c); + return; + } c[size] = '\0'; #if OCCT_BINARY_FILE_DO_INVERSEsrc/FoundationClasses/TKernel/FSD/FSD_File.cxx (3)
765-1209: Add success returns to Storage_Error writersSeveral
Storage_Errorhelpers now fall off the end without returning anything on success, introducing undefined behaviour. Each of these must returnStorage_VSOkwhen the stream stays healthy.Storage_Error FSD_File::BeginWriteCommentSection() { myStream << "BEGIN_COMMENT_SECTION\n"; if (myStream.bad()) return Storage_VSWriteError; + return Storage_VSOk; } @@ Storage_Error FSD_File::EndWriteCommentSection() { myStream << "END_COMMENT_SECTION\n"; if (myStream.bad()) return Storage_VSWriteError; + return Storage_VSOk; } @@ Storage_Error FSD_File::BeginWriteTypeSection() { myStream << "BEGIN_TYPE_SECTION\n"; if (myStream.bad()) return Storage_VSWriteError; + return Storage_VSOk; } @@ Storage_Error FSD_File::EndWriteTypeSection() { myStream << "END_TYPE_SECTION\n"; if (myStream.bad()) return Storage_VSWriteError; + return Storage_VSOk; } @@ Storage_Error FSD_File::BeginWriteRootSection() { myStream << "BEGIN_ROOT_SECTION\n"; if (myStream.bad()) return Storage_VSWriteError; + return Storage_VSOk; } @@ Storage_Error FSD_File::EndWriteRootSection() { myStream << "END_ROOT_SECTION\n"; if (myStream.bad()) return Storage_VSWriteError; + return Storage_VSOk; } @@ Storage_Error FSD_File::BeginWriteRefSection() { myStream << "BEGIN_REF_SECTION\n"; if (myStream.bad()) return Storage_VSWriteError; + return Storage_VSOk; } @@ Storage_Error FSD_File::EndWriteRefSection() { myStream << "END_REF_SECTION\n"; if (myStream.bad()) return Storage_VSWriteError; + return Storage_VSOk; } @@ Storage_Error FSD_File::BeginWriteDataSection() { myStream << "BEGIN_DATA_SECTION"; if (myStream.bad()) return Storage_VSWriteError; + return Storage_VSOk; }
823-834: Bail out when the section length read failsIf extracting
lenfails we set the error status but still callFlushEndOfLine()and iterate with an indeterminatelen. That mirrors the exception path from before and must return immediately after signaling the error.if (!(myStream >> len)) - SetErrorStatus(Storage_VSTypeMismatch); - - FlushEndOfLine(); + { + SetErrorStatus(Storage_VSTypeMismatch); + return; + } + + FlushEndOfLine();
905-1117: Avoid returning uninitialised counts
TypeSectionSize(),RootSectionSize(), andRefSectionSize()now continue after a failed extraction, so they return whatever junk happens to be ini. Return a safe value immediately when the read fails; callers will already see the status set.Standard_Integer FSD_File::TypeSectionSize() { Standard_Integer i; - if (!(myStream >> i)) - SetErrorStatus(Storage_VSTypeMismatch); + if (!(myStream >> i)) + { + SetErrorStatus(Storage_VSTypeMismatch); + return 0; + } FlushEndOfLine(); return i; } @@ Standard_Integer FSD_File::RootSectionSize() { Standard_Integer i; - if (!(myStream >> i)) - SetErrorStatus(Storage_VSTypeMismatch); + if (!(myStream >> i)) + { + SetErrorStatus(Storage_VSTypeMismatch); + return 0; + } FlushEndOfLine(); return i; } @@ Standard_Integer FSD_File::RefSectionSize() { Standard_Integer i; - if (!(myStream >> i)) - SetErrorStatus(Storage_VSTypeMismatch); + if (!(myStream >> i)) + { + SetErrorStatus(Storage_VSTypeMismatch); + return 0; + } FlushEndOfLine(); return i; }
♻️ Duplicate comments (2)
src/ModelingData/TKBRep/BinTools/BinTools_IStream.hxx (1)
116-122: Boolean read operations affected by Standard_Byte error handling issue.
ReadBool()(line 116) andoperator>>(Standard_Boolean&)(lines 118-122) both rely onReadByte(), which internally usesoperator>>(Standard_Byte&). As flagged in previous review comments, ifoperator>>(Standard_Byte&)doesn't setmyHasErroron read failure, these boolean read operations won't properly track errors either.This is a cascading impact of the issue already identified in the past review.
src/ApplicationFramework/TKStdL/StdLDrivers/StdLDrivers_DocumentRetrievalDriver.cxx (1)
199-204: Same error state reachability concern as lines 182-187.This segment has the same pattern and potential issues as lines 182-187: if
ReadPersistentObjectthrows exceptions caught byOCC_CATCH_SIGNALS, theErrorStatus()check at line 204 is unreachable. The past review specifically flagged this code section for dead error handling.The same recommendations apply:
- Remove
OCC_CATCH_SIGNALSif exceptions are no longer thrown- Clear error status before the operation:
aFileDriver->ClearErrorStatus()- Verify
ReadPersistentObjectsets error state properlyRun the following script to verify the error handling pattern in
ReadPersistentObject:#!/bin/bash # Search for ReadPersistentObject implementations ast-grep --pattern 'ReadPersistentObject($_) { $$$ }'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/ApplicationFramework/TKStd/StdStorage/StdStorage_RootData.cxx(2 hunks)src/ApplicationFramework/TKStdL/StdLDrivers/StdLDrivers_DocumentRetrievalDriver.cxx(2 hunks)src/FoundationClasses/TKernel/FSD/FSD_BinaryFile.cxx(23 hunks)src/FoundationClasses/TKernel/FSD/FSD_File.cxx(56 hunks)src/ModelingData/TKBRep/BinTools/BinTools_IStream.cxx(6 hunks)src/ModelingData/TKBRep/BinTools/BinTools_IStream.hxx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ModelingData/TKBRep/BinTools/BinTools_IStream.cxx
- src/ApplicationFramework/TKStd/StdStorage/StdStorage_RootData.cxx
🧰 Additional context used
🧬 Code graph analysis (2)
src/FoundationClasses/TKernel/FSD/FSD_File.cxx (2)
src/FoundationClasses/TKernel/Storage/Storage_RootData.cxx (2)
SetErrorStatus(141-144)SetErrorStatus(141-141)src/FoundationClasses/TKernel/FSD/FSD_BinaryFile.cxx (2)
IsEnd(130-133)IsEnd(130-130)
src/FoundationClasses/TKernel/FSD/FSD_BinaryFile.cxx (1)
src/FoundationClasses/TKernel/Storage/Storage_RootData.cxx (2)
SetErrorStatus(141-144)SetErrorStatus(141-141)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Prepare and Build on Ubuntu with Clang (x64)
- GitHub Check: Build Documentation
- GitHub Check: Prepare and Build on macOS with Clang (x64)
- GitHub Check: Prepare and Build on Windows with MSVC (x64)
- GitHub Check: Prepare and Build on macOS with Clang (No PCH)
🔇 Additional comments (2)
src/ModelingData/TKBRep/BinTools/BinTools_IStream.hxx (1)
62-73: LGTM! Well-designed error state management API.The
HasError()andClearError()methods provide a clean interface for error state management. The documentation is comprehensive and clearly explains usage patterns.src/ApplicationFramework/TKStdL/StdLDrivers/StdLDrivers_DocumentRetrievalDriver.cxx (1)
182-187: Clarify error handling intent: verify ifReadReferenceTypestill throws exceptions or only sets stateThe code pattern shows ambiguity about error handling behavior. Based on the codebase searches,
ReadReferenceTypeis a simple read operation, but the current implementation suggests conflicting error strategies:
If
ReadReferenceTypestill throws exceptions: TheErrorStatus()check at line 187 becomes unreachable dead code (exception would propagate out of theOCC_CATCH_SIGNALSblock, same issue flagged in the previous review).If
ReadReferenceTypeno longer throws (per PR objectives): TheOCC_CATCH_SIGNALSwrapper becomes unnecessary defensive code and should be removed.Error state management: No
ClearErrorStatus()calls are found before operations, creating risk of detecting stale errors from previous operations rather than errors from the currentReadReferenceTypecall.Required clarification:
- Confirm whether
ReadReferenceTypeandReadPersistentObjectcan throw exceptions (directly or indirectly)- If they don't throw, remove the
OCC_CATCH_SIGNALSwrappers- If they do throw, remove the
ErrorStatus()checks (they're unreachable)- Consider adding error state clearing before operations to ensure fresh state detection
Compilation Fixes:
- FSD_File.cxx: Added return Storage_VSOk statements to 9 methods that were missing returns in success paths
(BeginWriteCommentSection, EndWriteCommentSection, BeginWriteNameSection, EndWriteNameSection,
BeginWriteTypeSection, EndWriteTypeSection, BeginWriteRootSection, EndWriteRootSection,
BeginWriteDataSection)
CodeRabbit AI Critical Issue Fixes:
1. Removed unused exception includes (StdLDrivers_DocumentRetrievalDriver.cxx):
- Deleted Storage_StreamTypeMismatchError.hxx
- Deleted Storage_StreamFormatError.hxx
- Deleted Storage_StreamReadError.hxx
These are no longer needed after exception handling removal
2. Fixed uninitialized variable usage (FSD_File.cxx):
- ReadComment: Initialize len=0 and return immediately on read failure
- TypeSectionSize: Initialize i=0 and return 0 immediately on read failure
- RootSectionSize: Initialize i=0 and return 0 immediately on read failure
- RefSectionSize: Initialize i=0 and return 0 immediately on read failure
Prevents using garbage values when stream reads fail
3. Fixed data corruption in FSD_BinaryFile.cxx read methods:
- ReadString: Free buffer, clear output, and return immediately on fread failure
- ReadExtendedString: Free buffer, clear output, and return immediately on fread failure
Prevents assigning uninitialized memory to output strings
4. Fixed dead error handling code:
- StdStorage.cxx: Check driver->ErrorStatus() instead of unconditionally setting Storage_VSOk
after ReadReferenceType and ReadPersistentObject calls
Previously errors were masked by unconditional success assignment
5. Added missing driver error checks (StdStorage_HeaderData.cxx):
- Check driver error status after ReadInfo, ReadComment, WriteInfo, WriteComment
- Return immediately with proper error status if driver reports failure
Prevents silent failures where operations fail but callers believe they succeeded
These fixes address data corruption risks, uninitialized variable usage, and ensure
errors are properly propagated through the error state management system.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…ptions This commit completes the exception-to-error-state refactoring by removing the final 2 catch blocks: 1. Storage_TypeData.cxx (line 57): - Replaced catch (Storage_StreamTypeMismatchError) with driver->ErrorStatus() check - After ReadTypeInformations call, now checks driver error status - Returns immediately with proper error context if driver reports failure 2. Storage_Schema.cxx (line 463): - Replaced try-catch (Storage_StreamWriteError) wrapping entire write sequence - Removed try block (kept OCC_CATCH_SIGNALS for signal handling) - Added error status check after EndWriteDataSection - Checks driver->ErrorStatus() and propagates to aData with error context Also cleaned up 5 unused exception includes: - Storage_TypeData.cxx: Removed Storage_StreamTypeMismatchError.hxx - Storage_Schema.cxx: Removed Storage_StreamWriteError.hxx - Storage_HeaderData.cxx: Removed Storage_StreamTypeMismatchError.hxx and Storage_StreamExtCharParityError.hxx - StdStorage_RootData.cxx: Removed Storage_StreamTypeMismatchError.hxx With these changes: - Zero throws of Storage_Stream* exceptions remain in .cxx files - Zero catches of Storage_Stream* exceptions remain in .cxx files - All error handling now uses error state management via Storage_Error enum - Exception classes remain in headers but are marked deprecated for backwards compatibility
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/FoundationClasses/TKernel/Storage/Storage_BaseDriver.hxx (1)
196-220: Error state management API is well-designed.The new error state management methods provide a clean interface for transitioning from exception-based to status-based error handling:
- Clear separation between public query methods and protected setter
ClearErrorStatus()atomically resets both error code and extensionSetErrorStatus()allows setting error with optional contextConsider documenting thread-safety expectations for the error state. The current implementation stores error status in instance variables without synchronization. If concurrent usage is not intended (likely the case for file-based drivers), document that driver instances should not be shared across threads. If concurrent usage is expected, consider thread-safe error state management.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/ApplicationFramework/TKStd/StdStorage/StdStorage.cxx(2 hunks)src/ApplicationFramework/TKStd/StdStorage/StdStorage_RootData.cxx(2 hunks)src/ApplicationFramework/TKStd/StdStorage/StdStorage_TypeData.cxx(0 hunks)src/FoundationClasses/TKernel/Storage/Storage_BaseDriver.hxx(2 hunks)src/FoundationClasses/TKernel/Storage/Storage_HeaderData.cxx(2 hunks)src/FoundationClasses/TKernel/Storage/Storage_RootData.cxx(0 hunks)src/FoundationClasses/TKernel/Storage/Storage_Schema.cxx(1 hunks)src/FoundationClasses/TKernel/Storage/Storage_TypeData.cxx(1 hunks)src/ModelingData/TKBRep/BinTools/BinTools_IStream.hxx(2 hunks)
💤 Files with no reviewable changes (2)
- src/FoundationClasses/TKernel/Storage/Storage_RootData.cxx
- src/ApplicationFramework/TKStd/StdStorage/StdStorage_TypeData.cxx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Prepare and Build on Windows with MSVC (x64)
- GitHub Check: Prepare and Build on macOS with Clang (x64)
- GitHub Check: Prepare and Build on macOS with Clang (No PCH)
- GitHub Check: Prepare and Build on Ubuntu with Clang (x64)
- GitHub Check: Build Documentation
🔇 Additional comments (10)
src/ApplicationFramework/TKStd/StdStorage/StdStorage_RootData.cxx (2)
52-62: LGTM: Error status checking correctly implemented for Read path.The error handling refactor is properly implemented. After calling
ReadRoot, the code now checkstheDriver->ErrorStatus()and propagates any errors to the instance's error state with appropriate context. The fail-fast behavior (immediate return on error) is correct.
100-110: LGTM: Previously identified critical issue has been resolved.The missing error status check after
WriteRoot(flagged in the past review) has been properly addressed. The implementation now correctly checkstheDriver->ErrorStatus()after each write operation and propagates errors appropriately, matching the pattern used in the Read path.src/ModelingData/TKBRep/BinTools/BinTools_IStream.hxx (3)
62-68: LGTM! HasError() method is well-designed.The method is properly documented, const-correct, and provides clear guidance on error detection and handling. The inline implementation is efficient and appropriate.
70-74: LGTM! ClearError() method is properly implemented.The method correctly resets the error state and is well-documented with clear guidance on when to use it for error recovery and stream reuse.
153-153: LGTM! Private member is properly declared.The myHasError member is correctly encapsulated and serves as the foundation for error state management.
src/FoundationClasses/TKernel/Storage/Storage_HeaderData.cxx (1)
48-66: No changes required — error handling pattern is sound.The verification confirms the code correctly handles errors. OCC_CATCH_SIGNALS is a defensive mechanism for signal-to-exception conversion (when OCC_CONVERT_SIGNALS is defined); otherwise it expands to nothing. Its purpose is handling unexpected C-style signals, not normal I/O errors.
The actual error detection relies on the
theDriver->ErrorStatus()check immediately afterReadInfo(). Earlier analysis of the driver code (FSD_File.cxx, FSD_BinaryFile.cxx) shows thatSetErrorStatus()is consistently called for all error conditions (I/O failures, type mismatches, format errors). The deprecatedStorage_Stream*exception macros confirm this is the intended pattern: error state management instead of exception-throwing. No actual exception throws occur in current driver implementations.src/ApplicationFramework/TKStd/StdStorage/StdStorage.cxx (2)
127-134: LGTM! Error status checking is correctly implemented.The refactoring from exception handling to status checking for
ReadReferenceTypeis correct. The error status is captured and checked immediately after the operation.
150-157: LGTM! Consistent error handling pattern.The error status checking after
ReadPersistentObjectfollows the same correct pattern used throughout this file. The refactoring maintains proper error propagation.src/FoundationClasses/TKernel/Storage/Storage_BaseDriver.hxx (1)
293-294: LGTM! Data members correctly support the error state API.The private members
myErrorStatusandmyErrorStatusExtappropriately back the error state management functionality. The initialization toStorage_VSOk(mentioned in the AI summary for the .cxx file) ensures drivers start in a clean state.src/FoundationClasses/TKernel/Storage/Storage_TypeData.cxx (1)
56-61: Error status propagation implementation is correct, but caller discipline cannot be verified.The code correctly checks the driver's error status after
ReadTypeInformations()and properly propagates it. However, the call sites ofStorage_TypeData::Read()could not be definitively located in the codebase search, making it impossible to verify whether callers clear the driver's error status before invokingRead(). If stale error states from previous operations persist, the error check at line 56 could incorrectly detect them.Manually verify that all callers of
Storage_TypeData::Read()invokeClearErrorStatus()on the driver (or equivalent cleanup) before the call to ensure error state isolation.
During validation review, discovered that error status checks were incorrectly placed INSIDE the OCC_CATCH_SIGNALS blocks instead of outside them. Error checks should be performed AFTER the signal-catching scope completes, not within it. This ensures the error status is checked after the operation has fully completed and any signals have been handled. Fixed locations: - ReadInfo: Moved error check from inside (line 60-65) to outside (line 60-65) the OCC_CATCH_SIGNALS block - ReadComment: Moved error check from inside (line 87-92) to outside (line 87-92) the OCC_CATCH_SIGNALS block This makes Storage_HeaderData.cxx consistent with the pattern used in StdStorage_HeaderData.cxx, StdStorage.cxx, and StdLDrivers_DocumentRetrievalDriver.cxx where all error checks are placed after the closing brace of OCC_CATCH_SIGNALS blocks.
…agement Addresses CodeRabbit nitpick comment about documenting thread-safety expectations. Added @warning documentation to clarify that: - Storage_BaseDriver instances are NOT thread-safe - Driver instances should not be shared across threads - Each thread should use its own driver instance for file I/O operations This is consistent with the design of the Storage drivers which wrap file handles (FILE*) that are inherently not thread-safe. The typical usage pattern is sequential: open → read/write → close. Rationale: - File I/O operations are sequential by nature - Underlying file handles (FILE*, std::fstream) are not thread-safe - Adding synchronization would add unnecessary overhead for typical use cases - Multi-threaded applications can simply use separate driver instances per thread
1. Storage_Schema.cxx - Fix incorrect error context reporting:
CRITICAL ISSUE: Previously, error status was checked only once at the end
of all write operations. If an early operation (e.g., BeginWriteInfoSection)
failed, execution continued through all subsequent operations, and errorContext
kept getting overwritten. By the time the error was checked, errorContext
would incorrectly show "EndWriteDataSection" instead of the actual failing
operation.
FIX: Created CHECK_DRIVER_ERROR_AND_RETURN_IF_FAILED() macro that:
- Checks driver error status immediately after each operation
- Sets aData error status and extension with correct context
- Cleans up resources (iData->Clear(), Clear())
- Returns early to prevent cascading failures
Added checks after:
- BeginWriteInfoSection, WriteInfo, EndWriteInfoSection
- BeginWriteCommentSection, WriteComment, EndWriteCommentSection
- BeginWriteTypeSection, WriteTypeInformations (loop), EndWriteTypeSection
- BeginWriteRootSection, WriteRoot (loop), EndWriteRootSection
- BeginWriteRefSection, WriteReferenceType (loop), EndWriteRefSection
- BeginWriteDataSection, EndWriteDataSection
Benefits:
- Correct error context reporting
- Prevents undefined behavior from continuing after failures
- Avoids data corruption from dependent operations
- Early exit saves resources
2. BinTools_IStream.cxx - Fix operator bool() inconsistency:
CRITICAL ISSUE: operator bool() only checked underlying stream validity
(*myStream) but ignored myHasError flag. This created inconsistency where
"if (stream)" could evaluate true while stream.HasError() was also true,
causing confusion and potential bugs.
FIX: Changed from:
return *myStream ? Standard_True : Standard_False;
To:
return (*myStream && !myHasError) ? Standard_True : Standard_False;
Now operator bool() returns false when EITHER:
- The underlying stream is invalid, OR
- The myHasError flag is set
This maintains consistency with HasError() and provides correct
stream state information to callers.
Both issues identified by CodeRabbit AI code review.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/FoundationClasses/TKernel/Storage/Storage_HeaderData.cxx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Prepare and Build on macOS with Clang (No PCH)
- GitHub Check: Prepare and Build on Ubuntu with Clang (x64)
- GitHub Check: Prepare and Build on Windows with MSVC (x64)
- GitHub Check: Prepare and Build on macOS with Clang (x64)
- GitHub Check: Build Documentation
🔇 Additional comments (1)
src/FoundationClasses/TKernel/Storage/Storage_HeaderData.cxx (1)
82-92: Consistent error handling pattern applied.The error handling for
ReadCommentcorrectly mirrors the pattern used forReadInfo(lines 47-65), maintaining consistency across the refactoring. The same signal handler integration verification mentioned above applies here as well.
| { | ||
| try | ||
| { | ||
| OCC_CATCH_SIGNALS | ||
| theDriver->ReadInfo(myNBObj, | ||
| myStorageVersion, | ||
| myDate, | ||
| mySchemaName, | ||
| mySchemaVersion, | ||
| myApplicationName, | ||
| myApplicationVersion, | ||
| myDataType, | ||
| myUserInfo); | ||
| } | ||
| catch (Storage_StreamTypeMismatchError const&) | ||
| { | ||
| myErrorStatus = Storage_VSTypeMismatch; | ||
| myErrorStatusExt = "ReadInfo"; | ||
| return Standard_False; | ||
| } | ||
| catch (Storage_StreamExtCharParityError const&) | ||
| { | ||
| myErrorStatus = Storage_VSExtCharParityError; | ||
| myErrorStatusExt = "ReadInfo"; | ||
| return Standard_False; | ||
| } | ||
| OCC_CATCH_SIGNALS | ||
| theDriver->ReadInfo(myNBObj, | ||
| myStorageVersion, | ||
| myDate, | ||
| mySchemaName, | ||
| mySchemaVersion, | ||
| myApplicationName, | ||
| myApplicationVersion, | ||
| myDataType, | ||
| myUserInfo); | ||
| } | ||
|
|
||
| if (theDriver->ErrorStatus() != Storage_VSOk) | ||
| { | ||
| myErrorStatus = theDriver->ErrorStatus(); | ||
| myErrorStatusExt = "ReadInfo"; | ||
| return Standard_False; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify signal handler integration with error state management.
The refactoring from exception-based to status-based error handling looks correct and aligns with the PR objectives. However, ensure that when OCC_CATCH_SIGNALS catches a system signal (e.g., SIGSEGV, SIGFPE), the signal handler properly sets theDriver's error status. Otherwise, the subsequent ErrorStatus() check on line 60 might incorrectly pass even when a signal occurred.
🏁 Script executed:
#!/bin/bash
# Description: Verify OCC_CATCH_SIGNALS behavior with driver error state
# Search for OCC_CATCH_SIGNALS macro definition to understand signal handling
rg -n "define OCC_CATCH_SIGNALS" -A 10
# Search for signal handler implementations that might set driver error state
rg -n "SetErrorStatus" --type=cpp -C 3 -g "*Signal*" -g "*signal*"Length of output: 2606
Based on the search results, I now have enough information about how OCC_CATCH_SIGNALS works in OCCT. The critical documentation from OCCT's OCC_CATCH_SIGNALS macro makes a long jump to the nearest invocation when a signal occurs, and only there the C++ exception gets actually thrown. However, I need to examine the actual code context to assess the full risk. Let me search the codebase for how this code is called.
[/run_scripts]
Cannot verify integration of error state management with signal handling without examining the complete calling context and function definitions.
The original review comment identifies a genuine architectural concern: when a signal is caught by OCC_CATCH_SIGNALS, a long jump occurs and a C++ exception is thrown, but the refactored code pattern wraps ReadInfo() in OCC_CATCH_SIGNALS and then immediately checks theDriver->ErrorStatus() without a try-catch block. If an exception is thrown after a signal is caught, it will propagate unhandled rather than allowing the ErrorStatus() check on line 60 to execute.
However, the actual behavior depends on:
- Whether
ReadInfo()actually throws exceptions or only uses status codes - Whether the calling context has exception handlers
- The platform configuration (Windows vs. Linux OCC_CONVERT_SIGNALS behavior)
These cannot be conclusively determined from the provided context.
🏁 Script executed:
#!/bin/bash
# Look at full context around Storage_HeaderData.cxx lines 30-100 to see surrounding try-catch blocks
cat src/FoundationClasses/TKernel/Storage/Storage_HeaderData.cxx | head -110 | tail -80Length of output: 1970
🏁 Script executed:
#!/bin/bash
# Search for where this function is called to see if there's exception handling at call sites
rg -n "Storage_HeaderData" --type=cpp -C 5 | head -100Length of output: 8298
🏁 Script executed:
#!/bin/bash
# Check if ReadInfo() throws exceptions or is exception-free
rg -n "\.ReadInfo\(" --type=cpp -C 3 | head -60Length of output: 0
Let me search for the implementation of ReadInfo() and the function context containing this code.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Find the function name containing lines 47-65
rg -n "ReadInfo\|ReadComment" src/FoundationClasses/TKernel/Storage/Storage_HeaderData.cxx -B 5 | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for ReadInfo() method implementations in driver classes
rg -n "void.*ReadInfo" --type=cpp -C 2 | head -80Length of output: 2862
🏁 Script executed:
#!/bin/bash
# Look for function that calls Storage_HeaderData to see if exception handling wraps it
rg -n "ReadHeaderData" src/FoundationClasses/TKernel/FSD/FSD_BinaryFile.cxx -A 30 | head -50Length of output: 2030
Let me search for the actual implementation of ReadInfo() to see if it contains throw statements.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Get ReadInfo implementation in FSD_BinaryFile.cxx, lines 575-650
sed -n '575,650p' src/FoundationClasses/TKernel/FSD/FSD_BinaryFile.cxxLength of output: 2453
🏁 Script executed:
#!/bin/bash
# Search for throw statements in ReadInfo implementations
rg -n "throw" src/FoundationClasses/TKernel/FSD/FSD_BinaryFile.cxx | head -30Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find callers of Read() function (which calls ReadHeaderData)
rg -n "FSD_BinaryFile::Read\(" src/FoundationClasses/TKernel/FSD/FSD_BinaryFile.cxx -B 2 -A 50 | head -100Length of output: 0
Based on my analysis of the code:
Refactoring introduces unhandled exception propagation when system signals occur.
The ReadInfo() implementation itself doesn't throw exceptions in normal operation - it just reads from the stream. However, the refactoring has a critical architectural flaw:
When OCC_CATCH_SIGNALS catches a system signal (via setjmp/longjmp), it re-raises Standard_Failure inside the macro scope. This re-raised exception propagates upward because there is no try-catch block to handle it. The subsequent ErrorStatus() check on line 60 is never reached.
The pattern is fundamentally incompatible with the signal-handling semantics of OCC_CATCH_SIGNALS. The original code likely used explicit try-catch blocks to handle exception types like Storage_StreamTypeMismatchError, whereas the refactored code assumes exceptions won't propagate out of the scoped macro. This assumption fails if any signal occurs during file I/O operations.
This is safe only if:
- System signals are guaranteed not to occur during these operations, or
- Upper-level exception handlers catch
Standard_Failurefrom this code path
Neither guarantee appears to be enforced by the current implementation.
…toring OCC_CATCH_SIGNALS converts system signals to exceptions. Without try-catch blocks, these exceptions would propagate uncaught, making the macro unnecessary. Removed OCC_CATCH_SIGNALS from 17 locations across 9 files where no try-catch block exists: - StdStorage_HeaderData.cxx: 4 locations - StdStorage.cxx: 2 locations - StdStorage_RootData.cxx: 2 locations - StdStorage_TypeData.cxx: 2 locations - StdLDrivers_DocumentRetrievalDriver.cxx: 2 locations - Storage_TypeData.cxx: 1 location - Storage_RootData.cxx: 1 location - Storage_HeaderData.cxx: 2 locations - Storage_Schema.cxx: 1 location Preserved 2 instances that ARE inside try-catch blocks: - StdStorage.cxx line 62: Inside try-catch for Standard_Failure - StdLDrivers_DocumentRetrievalDriver.cxx line 80: Inside try-catch for Standard_Failure These try-catch blocks handle Standard_Failure (not Storage exceptions) and still need OCC_CATCH_SIGNALS to convert system signals to catchable exceptions.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ModelingData/TKBRep/BinTools/BinTools_IStream.cxx (1)
133-157: Guard byte-order inversion against read failures.The
operator>>overloads forStandard_Real(line 139) andStandard_Integer(line 154) perform byte-order inversion even when the read operation fails, potentially operating on uninitialized or garbage data. Theoperator>>(Standard_ShortReal&)implementation (lines 200-203) correctly guards the inversion withif (!myHasError).Apply this pattern consistently across all numeric types:
BinTools_IStream& BinTools_IStream::operator>>(Standard_Real& theValue) { if (!myStream->read((char*)&theValue, sizeof(Standard_Real))) myHasError = Standard_True; myPosition += sizeof(Standard_Real); #if DO_INVERSE - theValue = InverseReal(theValue); + if (!myHasError) + theValue = InverseReal(theValue); #endif return *this; }BinTools_IStream& BinTools_IStream::operator>>(Standard_Integer& theValue) { if (!myStream->read((char*)&theValue, sizeof(Standard_Integer))) myHasError = Standard_True; myPosition += sizeof(Standard_Integer); #if DO_INVERSE - theValue = InverseInt(theValue); + if (!myHasError) + theValue = InverseInt(theValue); #endif return *this; }src/FoundationClasses/TKernel/Storage/Storage_Schema.cxx (1)
481-492: Critical: Missing error check inside Write loop causes incorrect error reporting.The loop continues processing objects even after a
Write()failure. Unlike other loops in this function (lines 421–426, 438–443, 455–467), this loop lacks an immediate error check. IfWrite()fails:
- The error goes undetected until line 496
errorContextis overwritten to "EndWriteDataSection" (line 494), masking the true failure point- Subsequent objects are processed despite the failure
Apply this diff to add error checking inside the loop:
while (bit.More()) { p = bit.Value(); if (!p.IsNull()) { WFunc->Value(p->_typenum)->Write(p, theDriver, me); + CHECK_DRIVER_ERROR_AND_RETURN_IF_FAILED() p->_typenum = 0; } bit.Next(); }
🧹 Nitpick comments (1)
src/ModelingData/TKBRep/BinTools/BinTools_IStream.cxx (1)
163-177: Consider early exit on coordinate read failure and guard inversion.The
operator>>(gp_Pnt&)has two issues:
- Like the other operators, it performs byte-order inversion on potentially garbage data when a read fails (lines 170-172).
- The loop continues reading remaining coordinates even after a failure, potentially creating a partially valid/invalid point.
Consider breaking on first error and guarding the inversion:
BinTools_IStream& BinTools_IStream::operator>>(gp_Pnt& theValue) { Standard_Real aValue; for (int aCoord = 1; aCoord <= 3; aCoord++) { if (!myStream->read((char*)&aValue, sizeof(Standard_Real))) + { myHasError = Standard_True; + break; + } #if DO_INVERSE aValue = InverseReal(aValue); #endif theValue.SetCoord(aCoord, aValue); } myPosition += 3 * sizeof(Standard_Real); return *this; }Alternatively, if continuing to read all coordinates is intentional for consistent stream positioning, at minimum guard the inversion:
#if DO_INVERSE - aValue = InverseReal(aValue); + if (!myHasError) + aValue = InverseReal(aValue); #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/ApplicationFramework/TKStd/StdStorage/StdStorage.cxx(2 hunks)src/ApplicationFramework/TKStd/StdStorage/StdStorage_HeaderData.cxx(4 hunks)src/ApplicationFramework/TKStd/StdStorage/StdStorage_RootData.cxx(2 hunks)src/ApplicationFramework/TKStd/StdStorage/StdStorage_TypeData.cxx(0 hunks)src/ApplicationFramework/TKStdL/StdLDrivers/StdLDrivers_DocumentRetrievalDriver.cxx(2 hunks)src/FoundationClasses/TKernel/Storage/Storage_BaseDriver.hxx(2 hunks)src/FoundationClasses/TKernel/Storage/Storage_HeaderData.cxx(2 hunks)src/FoundationClasses/TKernel/Storage/Storage_RootData.cxx(0 hunks)src/FoundationClasses/TKernel/Storage/Storage_Schema.cxx(5 hunks)src/FoundationClasses/TKernel/Storage/Storage_TypeData.cxx(1 hunks)src/ModelingData/TKBRep/BinTools/BinTools_IStream.cxx(7 hunks)
💤 Files with no reviewable changes (2)
- src/FoundationClasses/TKernel/Storage/Storage_RootData.cxx
- src/ApplicationFramework/TKStd/StdStorage/StdStorage_TypeData.cxx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/ApplicationFramework/TKStd/StdStorage/StdStorage_RootData.cxx
- src/ApplicationFramework/TKStd/StdStorage/StdStorage.cxx
- src/ApplicationFramework/TKStdL/StdLDrivers/StdLDrivers_DocumentRetrievalDriver.cxx
- src/FoundationClasses/TKernel/Storage/Storage_HeaderData.cxx
🧰 Additional context used
🧬 Code graph analysis (2)
src/FoundationClasses/TKernel/Storage/Storage_Schema.cxx (1)
src/ApplicationFramework/TKStd/StdStorage/StdStorage_RootData.cxx (2)
Handle(98-101)Handle(98-98)
src/ModelingData/TKBRep/BinTools/BinTools_IStream.cxx (1)
src/FoundationClasses/TKernel/FSD/FSD_BinaryFile.cxx (2)
InverseShortReal(1695-1708)InverseShortReal(1695-1695)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Prepare and Build on macOS with Clang (No PCH)
- GitHub Check: Prepare and Build on Ubuntu with Clang (x64)
- GitHub Check: Prepare and Build on macOS with Clang (x64)
- GitHub Check: Prepare and Build on Windows with MSVC (x64)
- GitHub Check: Build Documentation
🔇 Additional comments (10)
src/ModelingData/TKBRep/BinTools/BinTools_IStream.cxx (4)
19-25: LGTM: Constructor initialization is correct.The
myHasErrormember is properly initialized toStandard_Falsein the member initialization list, consistent with the new error state management approach.
124-127: LGTM: Boolean conversion operator correctly checks both stream and error states.The operator now properly returns false if either the underlying stream has failed or the error flag is set, providing comprehensive error detection.
183-189: LGTM: Standard_Byte operator correctly sets error state.The implementation properly sets
myHasErroron read failure without additional operations that could be affected by garbage data.
195-205: LGTM: Standard_ShortReal operator correctly guards inversion.This implementation correctly sets the error flag on read failure and guards the byte-order inversion with
if (!myHasError)(lines 200-203), preventing undefined behavior when operating on uninitialized data. This pattern should be applied to the other numeric type operators.src/FoundationClasses/TKernel/Storage/Storage_Schema.cxx (2)
351-361: Excellent use of macro to reduce duplication.The macro effectively consolidates repetitive error checking logic while maintaining correct error propagation, context assignment, and cleanup. The approach significantly improves code maintainability.
505-506: Good macro hygiene.Properly undefining the macro prevents namespace pollution.
src/FoundationClasses/TKernel/Storage/Storage_BaseDriver.hxx (2)
196-223: Well-designed error state management API.The API provides clean separation between public read access and protected write access, enabling derived classes to set errors while allowing clients to check status. The thread-safety warning is valuable documentation. Inline implementations are appropriate for these simple accessors.
296-297: LGTM!The private members properly support the error state API.
src/FoundationClasses/TKernel/Storage/Storage_TypeData.cxx (1)
54-60: Consistent error handling implementation.The status check immediately after
ReadTypeInformationscorrectly propagates driver errors with appropriate context. This aligns with the project-wide migration to status-based error handling.src/ApplicationFramework/TKStd/StdStorage/StdStorage_HeaderData.cxx (1)
59-65: Consistent error handling across all operations.All four driver operations (
ReadInfo,ReadComment,WriteInfo,WriteComment) follow the same correct pattern: immediate status check after operation, proper error propagation with context, and early return on failure. This consistency improves code maintainability.Also applies to: 85-91, 132-138, 158-164
1. BinTools_IStream.cxx: Guard byte-order inversion against read failures
- operator>>(Standard_Real&): Only call InverseReal() if read succeeded
- operator>>(Standard_Integer&): Only call InverseInt() if read succeeded
- Previously, byte-order inversion was performed on uninitialized/garbage
data when reads failed, potentially causing undefined behavior
- Aligns with existing pattern used in operator>>(Standard_ShortReal&)
2. Storage_Schema.cxx: Add immediate error check in Write loop (line 489)
- Critical fix: Write() failures were not detected until line 497
- Error context was overwritten to "EndWriteDataSection" at line 495,
masking the true failure point ("Write")
- Now checks error status immediately after each Write() call
- Prevents processing subsequent objects after Write() failure
- Ensures accurate error reporting with correct operation context
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
Fixed critical oversight where exception handling was removed but error status
checks were not added. This caused errors to be silently ignored, potentially
leading to data corruption or incorrect program behavior.
Files fixed:
1. Storage_RootData.cxx (line 59-64):
- Added error check after ReadRoot() call
- Returns immediately with error status and context "ReadRoot"
2. StdStorage_TypeData.cxx (lines 58-63, 104-109):
- Added error check after ReadTypeInformations() call in Read()
- Added error check after WriteTypeInformations() call in Write()
- Both return immediately with error status and appropriate context
3. StdStorage.cxx (lines 280-282):
- Added error check after WriteReferenceType() call
- Returns immediately if write operation fails
- Prevents processing subsequent objects after failure
Pattern applied:
```cpp
theDriver->SomeOperation(...);
if (theDriver->ErrorStatus() != Storage_VSOk)
{
myErrorStatus = theDriver->ErrorStatus();
myErrorStatusExt = "OperationName";
return Standard_False; // or return anError for Storage_Error functions
}
```
This ensures that all operations that previously threw Storage exceptions now
properly check error status after transitioning to state-based error handling.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ApplicationFramework/TKStd/StdStorage/StdStorage_TypeData.cxx (1)
18-18: Remove unused include forStorage_StreamTypeMismatchError.hxx.This include is no longer needed since exception handling has been replaced with error state management. No references to
Storage_StreamTypeMismatchErrorexist in this file.Apply this diff to remove the unused include:
-#include <Storage_StreamTypeMismatchError.hxx>
🧹 Nitpick comments (3)
src/ApplicationFramework/TKStd/StdStorage/StdStorage.cxx (3)
32-34: Remove unused imports.These exception class imports are no longer needed after removing the try-catch blocks. The file now relies on error state checking via
theDriver->ErrorStatus().Apply this diff to remove the unused imports:
-#include <Storage_StreamTypeMismatchError.hxx> -#include <Storage_StreamFormatError.hxx> -#include <Storage_StreamWriteError.hxx>
127-129: Remove unnecessary braces.These braces are remnants from the removed try-catch block and now wrap only a single statement.
Apply this diff to clean up the code:
- { - theDriver->ReadReferenceType(aRef, aType); - } + theDriver->ReadReferenceType(aRef, aType);
149-151: Remove unnecessary braces.These braces are remnants from the removed try-catch block and now wrap only a single statement.
Apply this diff to clean up the code:
- { - aReadData.ReadPersistentObject(i); - } + aReadData.ReadPersistentObject(i);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/ApplicationFramework/TKStd/StdStorage/StdStorage.cxx(3 hunks)src/ApplicationFramework/TKStd/StdStorage/StdStorage_TypeData.cxx(2 hunks)src/FoundationClasses/TKernel/Storage/Storage_RootData.cxx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/FoundationClasses/TKernel/Storage/Storage_RootData.cxx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Documentation
- GitHub Check: Prepare and Build on Windows with MSVC (x64)
- GitHub Check: Prepare and Build on macOS with Clang (No PCH)
- GitHub Check: Prepare and Build on macOS with Clang (x64)
- GitHub Check: Prepare and Build on Ubuntu with Clang (x64)
🔇 Additional comments (3)
src/ApplicationFramework/TKStd/StdStorage/StdStorage.cxx (3)
131-133: LGTM! Error state checking correctly implemented.The error status check after
ReadReferenceTypecorrectly replaces the previous exception handling and ensures early return on failure.
153-155: LGTM! Error state checking correctly implemented.The error status check after
ReadPersistentObjectcorrectly replaces the previous exception handling and ensures early return on failure.
277-283: LGTM! Error state checking correctly implemented.The error status check after
WriteReferenceTypecorrectly replaces the previous exception handling. The braces grouping the write operation and error check are reasonable, though you could optionally remove them for consistency with other error checks in this file.
src/ApplicationFramework/TKStd/StdStorage/StdStorage_TypeData.cxx
Outdated
Show resolved
Hide resolved
src/ApplicationFramework/TKStd/StdStorage/StdStorage_TypeData.cxx
Outdated
Show resolved
Hide resolved
…toring
Cleaned up code remnants from try-catch block removal:
1. Removed unused include:
- StdStorage_TypeData.cxx: Removed Storage_StreamTypeMismatchError.hxx include
(no longer referenced after exception handling was removed)
2. Removed empty braces (vestigial try-catch block artifacts):
Storage (TKernel) files:
- Storage_TypeData.cxx: Removed braces around ReadTypeInformations() call
- Storage_RootData.cxx: Removed braces around ReadRoot() call
- Storage_HeaderData.cxx: Removed braces around ReadInfo() and ReadComment() calls
StdStorage (TKStd) files:
- StdStorage_TypeData.cxx: Removed braces around ReadTypeInformations() and
WriteTypeInformations() calls
- StdStorage_RootData.cxx: Removed braces around ReadRoot() and WriteRoot() calls
- StdStorage_HeaderData.cxx: Removed braces around ReadInfo(), ReadComment(),
WriteInfo(), and WriteComment() calls
- StdStorage.cxx: Removed braces around ReadReferenceType() call
These empty braces were left over when try-catch blocks were removed during the
transition from exception-based to state-based error handling. The braces served
no purpose after error status checks were added directly after driver method calls.
Pattern changed from:
```cpp
{
theDriver->SomeOperation(...);
}
if (theDriver->ErrorStatus() != Storage_VSOk) { ... }
```
To cleaner:
```cpp
theDriver->SomeOperation(...);
if (theDriver->ErrorStatus() != Storage_VSOk) { ... }
```
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/ApplicationFramework/TKStd/StdStorage/StdStorage_HeaderData.cxx (1)
14-18: Remove unused exception-related includes.After converting from exception-based to error-state-based error handling, the includes for
Standard_ErrorHandler.hxx,Storage_StreamTypeMismatchError.hxx, andStorage_StreamExtCharParityError.hxxare no longer referenced in the file and should be removed:-#include <Standard_ErrorHandler.hxx> #include <StdStorage_HeaderData.hxx> #include <Storage_BaseDriver.hxx> -#include <Storage_StreamTypeMismatchError.hxx> -#include <Storage_StreamExtCharParityError.hxx>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/ApplicationFramework/TKStd/StdStorage/StdStorage.cxx(3 hunks)src/ApplicationFramework/TKStd/StdStorage/StdStorage_HeaderData.cxx(4 hunks)src/ApplicationFramework/TKStd/StdStorage/StdStorage_RootData.cxx(2 hunks)src/ApplicationFramework/TKStd/StdStorage/StdStorage_TypeData.cxx(2 hunks)src/FoundationClasses/TKernel/Storage/Storage_HeaderData.cxx(2 hunks)src/FoundationClasses/TKernel/Storage/Storage_RootData.cxx(1 hunks)src/FoundationClasses/TKernel/Storage/Storage_TypeData.cxx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/FoundationClasses/TKernel/Storage/Storage_HeaderData.cxx
- src/FoundationClasses/TKernel/Storage/Storage_RootData.cxx
- src/ApplicationFramework/TKStd/StdStorage/StdStorage_RootData.cxx
- src/ApplicationFramework/TKStd/StdStorage/StdStorage.cxx
- src/ApplicationFramework/TKStd/StdStorage/StdStorage_TypeData.cxx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Prepare and Build on Ubuntu with Clang (x64)
- GitHub Check: Build Documentation
- GitHub Check: Prepare and Build on macOS with Clang (No PCH)
- GitHub Check: Prepare and Build on macOS with Clang (x64)
- GitHub Check: Prepare and Build on Windows with MSVC (x64)
🔇 Additional comments (4)
src/ApplicationFramework/TKStd/StdStorage/StdStorage_HeaderData.cxx (4)
48-63: LGTM! Error state management correctly implemented.The refactored error handling pattern is correct: unconditional call to
ReadInfo()followed by error status check viatheDriver->ErrorStatus(). If an error occurred, it's properly propagated tomyErrorStatusandmyErrorStatusExtis set to identify the operation.
80-87: LGTM! Consistent error handling pattern.The
ReadCommentsection follows the same correct pattern asReadInfo: unconditional operation followed by error status check and propagation. The implementation is consistent throughout the method.
117-132: LGTM! Write operations follow the same error handling pattern.The
WriteInfosection correctly implements error state management, consistent with the read operations. The unconditional call followed by error status check ensures proper error propagation.
149-156: LGTM! All sections consistently refactored.The
WriteCommentsection completes the consistent refactoring across all four operations. The error state management pattern is correctly and uniformly applied throughout bothRead()andWrite()methods.
…ring
After transitioning from exception-based to state-based error handling, removed
unused includes for Standard_ErrorHandler.hxx and Storage exception classes that
are no longer referenced in the code.
Files cleaned:
1. Storage (TKernel):
- Storage_BaseDriver.cxx: Removed Storage_StreamExtCharParityError.hxx
- Storage_HeaderData.cxx: Removed Standard_ErrorHandler.hxx
- Storage_RootData.cxx: Removed Standard_ErrorHandler.hxx, Storage_StreamTypeMismatchError.hxx
- Storage_Schema.cxx: Removed Standard_ErrorHandler.hxx
- Storage_TypeData.cxx: Removed Standard_ErrorHandler.hxx
2. StdStorage (TKStd):
- StdStorage_HeaderData.cxx: Removed Standard_ErrorHandler.hxx, Storage_StreamTypeMismatchError.hxx,
Storage_StreamExtCharParityError.hxx
- StdStorage_RootData.cxx: Removed Standard_ErrorHandler.hxx
- StdStorage_TypeData.cxx: Removed Standard_ErrorHandler.hxx
- StdStorage.cxx: Removed Storage_StreamTypeMismatchError.hxx, Storage_StreamFormatError.hxx,
Storage_StreamWriteError.hxx (kept Standard_ErrorHandler.hxx for Standard_Failure try-catch)
Rationale:
- Standard_ErrorHandler.hxx was only needed for try-catch blocks and OCC_CATCH_SIGNALS
- After removing exception handling for Storage exceptions, these includes serve no purpose
- StdStorage.cxx still uses Standard_ErrorHandler.hxx for Standard_Failure handling (preserved)
- This cleanup reduces compilation dependencies and improves code clarity
Updated deprecation marking for 5 Storage exception classes to use OCCT's Standard_DEPRECATED macro instead of simple @deprecated comments. This provides better compiler-level deprecation warnings and follows OCCT coding standards. Exception classes updated: - Storage_StreamTypeMismatchError.hxx - Storage_StreamExtCharParityError.hxx - Storage_StreamWriteError.hxx - Storage_StreamFormatError.hxx - Storage_StreamReadError.hxx Changes for each class: - Added @deprecated OCCT 7.9.0 comment with detailed explanation - Wrapped class declaration with Standard_DEPRECATED macro - Removed @deprecated comments from macro definitions - Kept DEFINE_STANDARD_EXCEPTION without deprecation marking Pattern: ```cpp //! @deprecated OCCT 7.9.0 - This exception is deprecated and no longer thrown. //! Error handling is now performed via Storage_Error enum and error state management //! through Storage_BaseDriver::ErrorStatus() instead of exceptions. class Standard_DEPRECATED( "This exception is no longer thrown; use Storage_BaseDriver::ErrorStatus() for error state management") Storage_StreamTypeMismatchError; ``` This follows the OCCT pattern used for deprecated exceptions like Transfer_TransferDeadLoop. The macro will generate compiler warnings when these classes are used, encouraging migration to the new error state management approach.
Fixed code formatting to match OCCT style guidelines:
1. Storage_Schema.cxx:
- Removed indentation from #define and #undef preprocessor directives
- Removed indentation from comments preceding macro definitions
- Preprocessor directives should start at column 0 per OCCT conventions
2. Exception header files (5 files):
- Split long Standard_DEPRECATED string across two lines
- Improved readability by breaking at natural boundary ("use ")
- Maintains 100-column line length limit
Files updated:
- Storage_StreamTypeMismatchError.hxx
- Storage_StreamExtCharParityError.hxx
- Storage_StreamWriteError.hxx
- Storage_StreamFormatError.hxx
- Storage_StreamReadError.hxx
Pattern changed:
Before:
class Standard_DEPRECATED(
"This exception is no longer thrown; use Storage_BaseDriver::ErrorStatus() for error state management")
After:
class Standard_DEPRECATED("This exception is no longer thrown; use "
"Storage_BaseDriver::ErrorStatus() for error state management")
2 participants
Replace exception-based error handling with error state management for Storage stream errors. This change eliminates throwing and catching of:
Changes:
The existing Storage_Error enum is used for error codes, maintaining compatibility with existing error handling patterns in OCCT.