Skip to content
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

WT-3720 flags macros cast flags to unsigned values, hiding warnings. #3789

Merged
merged 8 commits into from Dec 8, 2017

Conversation

keithbostic
Copy link
Contributor

@agorrod, @michaelcahill, based on the discussion in WT-3716, here's the first step.

If you agree this is a reasonable approach, I'll go ahead and do the rest of the work required for WT-3716 and WT-3720.

The reason for flags.py was to auto-generate flags values, but also to
handle the configuration problems we had in Berkeley DB where different
APIs had flags that shared a name. WiredTiger went a different direction
with APIs and we don't have flags to APIs that share a name. Continue to
auto-generate flags values, but move the flags into the #include files
with their fields.
@agorrod
Copy link
Member

agorrod commented Nov 15, 2017

@keithbostic this seems like a reasonable approach. I think it would be neater if the blocks of flags only had a starting marker, but it's safer the way you implemented it.

Could you outline your plan here. Specifically:

  • Will we switch all flags defines to use this form?
  • Will there be some way to mark flags that require 64 bits vs 32 bits?

@keithbostic
Copy link
Contributor Author

Will we switch all flags defines to use this form?

I think that’s an open question, I don’t feel strongly, I could be talked into doing them all or just where it’s needed. Since we have to declare them unsigned for clang, perhaps it’s easier to do them all?

Will there be some way to mark flags that require 64 bits vs. 32 bits?

I don’t think one is necessary. The Python script just uses however many bits it needs, and if we get the field declaration wrong and try to assign a 64 bit flag to a 32 bit field, the compiler will complain.

@agorrod
Copy link
Member

agorrod commented Nov 15, 2017

@keithbostic I've moved the discussion into the JIRA ticket.

…ipping

the btree in-memory key encoding magic, on-page cells, and the logging slot
magic for now.

We no longer need to cast F_XXX and FLD_XXX macro arguments, remove them.
@keithbostic
Copy link
Contributor Author

@agorrod, I've pushed the next step in this change, automatically generating most of the other flags in the source tree.

I skipped the logging slot bit masks because they're relatively tricky.

I didn't explicitly call out the places where a flag is placed on disk so we can't automatically generate it: let me know if you think that would be worth doing.

Assuming you approve this change, I think we can merge it, and then I'll do the remaining WT-3716 fixes in a separate branch. That said, I'd rather not merge this change until after we're done with the 3.6 branch, I think it's likely safe, but I'm not interested in finding out if I'm wrong.

Copy link
Member

@agorrod agorrod left a comment

Choose a reason for hiding this comment

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

@keithbostic I've got one suggestion, apart from that this change looks like a good step forward.

@@ -92,6 +118,12 @@ union __wt_lsn {
* a few special states, reserve the top few bits for state. That makes
* the maximum size less than 32 bits for both joined and released.
*/
/*
* XXX
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer if this had a more obvious name like FLD_LOG_SLOT_ISSET since it is doing something a bit odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer if this had a more obvious name like FLD_LOG_SLOT_ISSET since it is doing something a bit odd.

Agreed, pushed in a3d72e4.

usage, make it clear it's not a general-purpose macro.
@agorrod
Copy link
Member

agorrod commented Dec 8, 2017

Thanks @keithbostic lgtm.

@agorrod agorrod merged commit f307adf into develop Dec 8, 2017
@agorrod agorrod deleted the wt-3716-3720-flags-rework branch December 8, 2017 03:23
keithbostic added a commit that referenced this pull request Dec 17, 2017
…#3789)

Avoid casting the the flag macros - doing so can hide coding errors.

The reason for flags.py was to auto-generate flags values, but also to
handle the configuration problems we had in Berkeley DB where different
APIs had flags that shared a name. WiredTiger went a different direction
with APIs and we don't have flags to APIs that share a name. Continue to
auto-generate flags values, but move the flags into the #include files
with their fields.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants