Skip to content

Fix MapBatchTest#761

Merged
gdbelvin merged 7 commits intogoogle:masterfrom
gdbelvin:fix/new_nodeid
Jul 28, 2017
Merged

Fix MapBatchTest#761
gdbelvin merged 7 commits intogoogle:masterfrom
gdbelvin:fix/new_nodeid

Conversation

@gdbelvin
Copy link
Copy Markdown
Contributor

@gdbelvin gdbelvin commented Jul 27, 2017

Fixed the following problems:

  • NewNodeIDFromRelativeBitInt

    • Was placing subtree indexes in a left justified rather than right
      justified manner.
  • NodeID.Split

    • Modular arithmetic for suffix.Bits assumed equal strata levels

@gdbelvin gdbelvin added the bug label Jul 27, 2017
@gdbelvin gdbelvin force-pushed the fix/new_nodeid branch 2 times, most recently from 81f90e7 to 8a869b8 Compare July 27, 2017 15:34
@gdbelvin gdbelvin requested review from AlCutter and Martin2112 July 27, 2017 15:59
@gdbelvin gdbelvin requested review from RJPercival and pphaneuf July 28, 2017 12:46
if err := verifyGetMapLeavesResponse(getResp, indexes, int64(numBatches),
pubKey, hasher, tree.TreeId); err != nil {
return err
return fmt.Errorf("batch: %v, %v", i, err)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

fmt.Errorf("batch %v: verifyGetMapLeavesResponse() = %v", i, err) might be a bit quicker at pointing the person debugging it here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

also at L180

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

}
}

// h2b converts a hex string into []byte.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There's testonly.MustHexDecode which does the same thing, BTW.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Shorter names are nice?

Copy link
Copy Markdown
Member

@AlCutter AlCutter Jul 28, 2017

Choose a reason for hiding this comment

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

So is deduplication :)
How about this:

var h2b = testonly.MustHexDecode

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ohhhh, I like!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

storage/types.go Outdated
if bits == 0 {
panic(fmt.Sprintf("storage Split: %x(n.PrefixLenBits: %v - prefixBytes: %v *8) == 0", n.Path, n.PrefixLenBits, prefixBytes))
}
suffixBytes := (bits + 7) / 8
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you can use bytesForBits for this :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

He might have removed that in a previous PR?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It lives on (at least currently):

func bytesForBits(numBits int) int {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

gdbelvin added 6 commits July 28, 2017 15:29
Fixed the following problems:

- NewNodeIDFromRelativeBitInt
  - Was placing subtree indexes in a left justified rather than right
    justified manner.

- NodeID.Split
  - Modular arithmatic for suffix.Bits assumed equal strata levels
@gdbelvin gdbelvin merged commit 3349aab into google:master Jul 28, 2017
@gdbelvin gdbelvin deleted the fix/new_nodeid branch July 28, 2017 16:48
@gdbelvin gdbelvin mentioned this pull request Aug 9, 2017
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.

5 participants