FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix and simplify DirectoryEntry validation by jeremy-visionaid · Pull Request #181 · openmcdf/openmcdf · GitHub

Fix and simplify DirectoryEntry validation - #181

Merged
ironfede merged 2 commits into
openmcdf:masterfrom
Visionaid-International-Ltd:fix-duplicate-sid-check
Oct 7, 2024
Merged

ironfede merged 2 commits into
openmcdf:masterfrom
Visionaid-International-Ltd:fix-duplicate-sid-check

Conversation

Copy link
Copy Markdown
Collaborator

Ensure both the root and left sibilings are considered when checking for duplicate SIDs.

Also, remove redundant lookups.

jeremy-visionaid force-pushed the fix-duplicate-sid-check branch from ba57c3a to 877ba99 Compare October 6, 2024 22:45

Copy link
Copy Markdown
Collaborator Author

@ironfede Looks to me like the root should also optionally throw if ValidationExceptionEnabled is set? i.e.

        private void LoadChildren(RBTree bst, IDirectoryEntry de)
        {
            if (de.Child != DirectoryEntry.NOSTREAM)
            {
                IDirectoryEntry child = directoryEntries[de.Child];
                // if (child.StgType != StgType.StgInvalid) // Should actually throw?
                {
                    List<int> levelSIDs = new List<int>();
                    LoadChildren(bst, child, levelSIDs);
                }
            }
        }

WDYT?

ironfede left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@jeremy-visionaid I think thats named used in methods are not 100% representative or their function. For example SiblingIsValid seems to imply it checks for "validity" but it really checks for "existance" of sibling AND do validation. So I think it should be renamed "HasSiblings" and internally throws for validity exception.

ironfede commented Oct 7, 2024

Copy link
Copy Markdown
Collaborator

@ironfede Looks to me like the root should also optionally throw if ValidationExceptionEnabled is set? i.e.

        private void LoadChildren(RBTree bst, IDirectoryEntry de)
        {
            if (de.Child != DirectoryEntry.NOSTREAM)
            {
                IDirectoryEntry child = directoryEntries[de.Child];
                // if (child.StgType != StgType.StgInvalid) // Should actually throw?
                {
                    List<int> levelSIDs = new List<int>();
                    LoadChildren(bst, child, levelSIDs);
                }
            }
        }

WDYT?

I think it should throw.

Ensure both the root and left sibilings are considered when checking
for duplicate SIDs.

Also, remove redundant lookups.

Copy link
Copy Markdown
Collaborator Author

@jeremy-visionaid I think thats named used in methods are not 100% representative or their function. For example SiblingIsValid seems to imply it checks for "validity" but it really checks for "existance" of sibling AND do validation. So I think it should be renamed "HasSiblings" and internally throws for validity exception.

Hmm, I might take it further and say that the method actually isn't even aware of sibilings - only the caller is, so "HasSiblings" is also arguably misleading. I think perhaps I could just rewrite it to avoid having a boolean return at all.

jeremy-visionaid force-pushed the fix-duplicate-sid-check branch from 877ba99 to 493f0d5 Compare October 7, 2024 07:58

Copy link
Copy Markdown
Collaborator Author

@ironfede I've rewriten to avoid having a bool method, and added another commit on top that fixes the case where an exception was not thrown if the root entry was invalid too.

jeremy-visionaid force-pushed the fix-duplicate-sid-check branch from 493f0d5 to e7919e3 Compare October 7, 2024 08:11
ironfede merged commit d9fbc1f into openmcdf:master Oct 7, 2024
jeremy-visionaid deleted the fix-duplicate-sid-check branch October 16, 2024 03:55
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL