Skip to content

Simplify Clumplet Reader/Writer + code analysis issues #8507

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Apr 11, 2025

Conversation

mrotteveel
Copy link
Member

Uses isTagged() instead of having three or four different ways of discerning tagged vs untagged (in some cases, missing some kinds). Some code cleanup, annotating [[noreturn]]/noexcept, addressing switch code analysis warnings.

Also fixes a bug where ClumpletWriter.reset(...) shouldn't work for tagged buffers (apparently not happening in production code?)

@mrotteveel mrotteveel requested a review from AlexPeshkoff April 8, 2025 07:19
@@ -437,6 +440,9 @@ void ClumpletWriter::insertBytesLengthCheck(UCHAR tag, const void* bytes, const
cur_offset += 4;
}
break;
default:
invalid_structure("invalid length size", lenSize);
Copy link
Member

Choose a reason for hiding this comment

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

lenSize is set only by our code, please remove this default label, use fb_assert() instead

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean fb_assert(false) here in the default branch, or adding fb_assert(lenSize >= 0 && lenSize <= 2 || lenSize == 4) earlier?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now I pushed a change with the second assert I mentioned in the switch; do you think that it's OK, or do you want it moved outside the switch, or as fb_assert(false)?

@AlexPeshkoff AlexPeshkoff self-assigned this Apr 9, 2025
@mrotteveel
Copy link
Member Author

@AlexPeshkoff Do you want me to make any other changes.

BTW: I suggest squash-merging this for a cleaner history.

@AlexPeshkoff
Copy link
Member

AlexPeshkoff commented Apr 11, 2025 via email

@TommiPrami
Copy link
Contributor

ClumpletWriter.reset(...)

From out of curiocity what/where this term "Clumpet" comes from and/or mean, in this context?

Google gave me an answers that where more confusing than informative 😄

@mrotteveel
Copy link
Member Author

mrotteveel commented Apr 11, 2025

ClumpletWriter.reset(...)

From out of curiocity what/where this term "Clumpet" comes from and/or mean, in this context?

Google gave me an answers that where more confusing than informative 😄

It's clumplet, not clumpet; I believe it is a word made up by Jim Starkey (not 100% sure though), and I think you should read it as "[a collection of] small clumps of bytes", as in "-let" being a diminutive, and clumps of bytes (compare clumps of clay) for the contents of the buffer. That said, I might just read too much in it.

@AlexPeshkoff
Copy link
Member

ClumpletWriter.reset(...)

From out of curiocity what/where this term "Clumpet" comes from and/or mean, in this context?
Google gave me an answers that where more confusing than informative 😄

It's clumplet, not clumpet; I believe it is a word made up by Jim Starkey (not 100% sure though), and I think you should read it as "[a collection of] small clumps of bytes", as in "-let" being a diminutive, and clumps of bytes (compare clumps of clay) for the contents of the buffer. That said, I might just read too much in it.

Not by Jim Starkey but by Nickolay Samofatov .

@dyemanov
Copy link
Member

ClumpletWriter.reset(...)

From out of curiocity what/where this term "Clumpet" comes from and/or mean, in this context?
Google gave me an answers that where more confusing than informative 😄

It's clumplet, not clumpet; I believe it is a word made up by Jim Starkey (not 100% sure though), and I think you should read it as "[a collection of] small clumps of bytes", as in "-let" being a diminutive, and clumps of bytes (compare clumps of clay) for the contents of the buffer. That said, I might just read too much in it.

Not by Jim Starkey but by Nickolay Samofatov .

Header page clumplets exist in the InterBase 6.0 source code, so I believe credits really belong to Jim ;-) Nickolay just extended the term to various API parameter buffers.

@mrotteveel
Copy link
Member Author

@AlexPeshkoff Anything else, or can I merge it?

@AlexPeshkoff
Copy link
Member

@AlexPeshkoff Anything else, or can I merge it?

OK to merge

@mrotteveel mrotteveel merged commit c74ca39 into FirebirdSQL:master Apr 11, 2025
23 of 24 checks passed
@mrotteveel mrotteveel deleted the simplify-clumplet-rw branch April 11, 2025 10:14
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.

4 participants