Skip to content

Improve code of class BePlusTree #8433

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 4 commits into from
Feb 10, 2025
Merged

Improve code of class BePlusTree #8433

merged 4 commits into from
Feb 10, 2025

Conversation

hvlad
Copy link
Member

@hvlad hvlad commented Feb 9, 2025

Maked it usabe with debugger, removed a lot of casts and a few other misc improvemens.

@hvlad hvlad self-assigned this Feb 9, 2025
@aafemt
Copy link
Contributor

aafemt commented Feb 9, 2025

Access to a member of unions that wasn't recently assigned is UB in C++. For reliability it would be better to remove void* pointer from NodePtr and make explicit separate constructors for node pointer and item pointer.

…ete instead of allocate/deallocate.

Removed excess "friend class" declarations.
NodePtr changed to avoid potential undefined behavior.
  removed Allocator from template arguments,
  removed pointer based ctor in favor of reference based one.
@hvlad hvlad merged commit 39803b5 into master Feb 10, 2025
48 checks passed
@hvlad
Copy link
Member Author

hvlad commented Feb 10, 2025

Should it be backported and, if yes, into which branches ?

@dyemanov
Copy link
Member

Usually, we don't backport refactoring commits. If you think it's really needed, I may agree with backporting into v5 only.

@hvlad
Copy link
Member Author

hvlad commented Feb 11, 2025

The main goal of this PR is not the refactoring. It is about debugging experience, which was close to impossible.

@AlexPeshkoff
Copy link
Member

I agree with Vlad - old artifacts, invented by skidder to support use of a tree in memory manager, cause a lot of problems with debugging. I see no problems backporting this as deep as possible - even to v.3.

@dyemanov
Copy link
Member

OK, I don't mind (except maybe v3 which is nearly end-of-life). I just suppose that the need of debugging there may be over-estimated, given the last bugfix in this class was about 20 years ago. And even if we have something fixed in master/v5, it can be easily backported "as is".

@hvlad
Copy link
Member Author

hvlad commented Feb 11, 2025

Let me clarify - it is not about debugging BePlusTree only, it is mostly about debugging everything else where it is used.
I.e. before this patch it was hard to impossible to inspect tree or map contents.

@AlexPeshkoff
Copy link
Member

@hvlad +1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants