Skip to content

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