Make the tellg failure check explicit and document the EPContext write gate error handling - #31196
Open
GopalakrishnanN wants to merge 1 commit into
Open
Make the tellg failure check explicit and document the EPContext write gate error handling#31196GopalakrishnanN wants to merge 1 commit into
GopalakrishnanN wants to merge 1 commit into
Conversation
…or_code handling Two follow-ups from automated review of the EPContext sample helper. First, the file-size read held tellg() in a std::streampos and compared it with 0, which works only through the implicit fpos to streamoff conversion; holding it in a std::streamoff makes that a plain integral comparison against the documented pos_type(-1) failure value. Second, a note on EnsureRegularFileForWrite explaining why a set error_code falls through to allow the write instead of failing closed. Verified on MSVC that a simply-missing target sets error_code (value 2) rather than only reporting file_type not_found, so failing closed on any error would reject the ordinary new-file write, which is the common path. Comparing against errc no_such_file_or_directory does not separate the cases either, since ERROR_INVALID_NAME maps to the same condition. A target whose type cannot be determined fails at the open regardless. No functional change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Two follow-ups from automated review of the EPContext sample helper, both no-functional-change.
1.
tellg()failure check. The file-size read held the result in astd::streamposand compared it with0, which works only through the implicitfpostostreamoffconversion. Holding it in astd::streamoffmakes it a plain integral comparison against the documentedpos_type(-1)failure value.2. Documented the
error_codehandling inEnsureRegularFileForWrite. A reviewer flagged that a failingsymlink_statusfalls through and skips the symlink check, and suggested failing closed. I verified the behavior before changing anything, and did not apply that fix, because it would break the common path:error_codefile_typenot_foundnot_foundnot_foundregularOn MSVC a simply-missing target sets
error_code, so failing closed on any error would reject ordinary new-file writes. Comparing againststd::errc::no_such_file_or_directorydoes not separate the cases either:ERROR_INVALID_NAMEmaps to that same condition, so a genuine error is indistinguishable from a benign absent path by either the code or the type.The residual exposure is small: a target whose type cannot be determined fails at the
ofstreamopen anyway. So the existing fall-through is deliberate, and this adds a short comment recording that rather than changing the guard.Motivation and Context
This file is sample/reference code EP authors are expected to copy, so a subtly non-obvious guard is worth documenting, and a comparison that relies on an implicit conversion is worth making explicit.
Follow-up to #29294. Raised by automated review on #31195, where the comments landed on a stale diff.
Verified:
clang-formatclean, within 120 columns,onnxruntime_autoep_testbuilds, and theEpContextDataUtilstests pass (11 passed, 2 skipped - both symlink tests, which need privileges on Windows).