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

Convert PropertyIdentifierAndOffset to a record struct by Numpsy · Pull Request #422 · openmcdf/openmcdf · GitHub

Convert PropertyIdentifierAndOffset to a record struct - #422

Merged
jeremy-visionaid merged 1 commit into
openmcdf:mainfrom
Numpsy:prop_ident_struct
Apr 27, 2026
Merged

jeremy-visionaid merged 1 commit into
openmcdf:mainfrom
Numpsy:prop_ident_struct

Conversation

Numpsy commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Instances are immutable (they're never modified after creation), so being a record seems reasonable and makes things a tad simpler.

Just a little bit of musing on things when testing other changes.

I initially just made it a record to make it immutable, but then ran the benchmarks being a struct does save a little bit of memory -

Before:

| Method                                   | Mean     | Error     | StdDev    | Gen0   | Allocated |
|----------------------------------------- |---------:|----------:|----------:|-------:|----------:|
| ReadSummaryInformation                   | 2.703 us | 0.1786 us | 0.0098 us | 0.1869 |   3.07 KB |
| ReadDocumentSummaryInformation           | 5.573 us | 0.3971 us | 0.0218 us | 0.3128 |    5.2 KB |
| ReadWinUnicodeDocumentSummaryInformation | 4.632 us | 0.4080 us | 0.0224 us | 0.3738 |   6.18 KB |

After:

| Method                                   | Mean     | Error     | StdDev    | Gen0   | Allocated |
|----------------------------------------- |---------:|----------:|----------:|-------:|----------:|
| ReadSummaryInformation                   | 2.627 us | 0.3772 us | 0.0207 us | 0.1678 |   2.79 KB |
| ReadDocumentSummaryInformation           | 5.670 us | 0.2162 us | 0.0119 us | 0.2899 |    4.8 KB |
| ReadWinUnicodeDocumentSummaryInformation | 4.653 us | 0.4106 us | 0.0225 us | 0.3510 |   5.73 KB |

Open for comments on it being a struct or a class. There are similar things in the core library that are structs for reference though.

Instances are immutable (they're never modified after creation), so being a record seems reasonable and makes things a tad simpler.
jeremy-visionaid changed the title Convert PropertyIdentifierAndOffset into a record Convert PropertyIdentifierAndOffset to a record struct Apr 27, 2026

jeremy-visionaid 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

I agree it should be a record struct. Minor nit that the commit message is incorrect since records and record structs are different

jeremy-visionaid merged commit 32cb53b into openmcdf:main Apr 27, 2026
2 checks passed
Numpsy deleted the prop_ident_struct branch April 28, 2026 08:23
This was referenced May 18, 2026
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