Refactor: move inode to internal/common package #407
Conversation
cmd/bbolt/main.go
Outdated
| // leafPageElement retrieves a leaf page element. | ||
| func (cmd *pageItemCommand) leafPageElement(pageBytes []byte, index uint16) (*guts_cli.LeafPageElement, error) { | ||
| p := (*guts_cli.Page)(unsafe.Pointer(&pageBytes[0])) | ||
| func (cmd *pageItemCommand) validateLeafPage(pageBytes []byte, index uint16) (*common.Page, error) { |
There was a problem hiding this comment.
The previous name suggested that it's a cheap ('getter type') call for accessing content of the page. And the santity checks were only used to rise a meaningful errors if something goes wrong...
The new name suggests it's rather a 'deep analysis' rather than the accessor . What was the motivation ?
There was a problem hiding this comment.
The previous (*pageItemCommand) leafPageElement actually includes two operations:
I just extract the first operation into an explict method, namely (*pageItemCommand) validateLeafPage.
The main motivation is to make sure all LeafPageElement[s] / BranchPageElement[s] have consistent meaning. It's a little confusing that some of them means just accessing content, but others means validating & accessing content.
There was a problem hiding this comment.
I don't consider these validations...
To me it's only a sanity check that confirms whether the method was used in a correct context.
We cannot access leaf elements on a page that is not a 'leaf' and we should go above the number of items on the page.
I would just add similar 'assertions / sanity checks' on other accessors to be on the 'defensive' side.
There was a problem hiding this comment.
assertion seems not good for CMD. Usually we need to return an error (display an error message) instead of just panicking for CMD coming from users.
Let me revert this change if you don't have objections.
There was a problem hiding this comment.
I recall the other motivation of extracting the validation into an explicit method.... previously (*pageItemCommand) leafPageElement returns *guts_cli.LeafPageElement, the struct guts_cli.LeafPageElement was exposed directly. In this PR, we only expose the method (*Page) LeafPageElement() instead of the struct leafPageElement, so we can't return common.leafPageElement any more.
Anyway, updated. Please take a look at the last commit f68adfe
| } | ||
| if p.Type() != "leaf" { | ||
| return nil, fmt.Errorf("leafPageElement: expected page type of 'leaf', but got '%s'", p.Type()) | ||
| if p.Typ() != "leaf" { |
There was a problem hiding this comment.
The existing bbolt package uses Typ, while the cmd/bbolt uses Type. Eventually I selected the former. It seems not a big deal. If you have big concern on this, then I can update it.
Personally I'd like Typ, because the there is an existing Type interface in reflect, and actually each value has a Type method as well. It isn't a problem technically, but I tend to use Typ to avoid any possible confusion.
| for i := 0; i < int(p.count); i++ { | ||
| elem := p.branchPageElement(uint16(i)) | ||
| tx.forEachPageInternal(append(pgidstack, elem.pgid), fn) | ||
| if (p.Flags() & common.BranchPageFlag) != 0 { |
There was a problem hiding this comment.
For follow up PR, but all such constructs (p.Flags() & common.BranchPageFlag)
should get changed to p.Flags().IsBranchPage()
There was a problem hiding this comment.
Yes, I have the same thought.
There was a problem hiding this comment.
Let me do it in a separate PR.
|
@ptabor do you have big concern or comment before I merge this PR? |
Points: 1. There are lots of duplicated definitions between bolt and guts_cli, which is definitely not good. 2. The implementation in guts_cli also has issue, please refer to #391. This refactoring can fix the issue. Signed-off-by: Benjamin Wang <wachao@vmware.com>
Signed-off-by: Benjamin Wang <wachao@vmware.com>
Signed-off-by: Benjamin Wang <wachao@vmware.com>
Signed-off-by: Benjamin Wang <wachao@vmware.com>
|
Thanks @ptabor |
This PR is on top of #394
The next step is to enhance the surgery command based on this PR to resolve the issue #402