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

Is there a bug in bit_array's sub function? #2506

Merged
merged 4 commits into from Oct 3, 2018

Conversation

james-ray
Copy link
Contributor

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md
    This is original code:
    if bA.Bits > o.Bits {
    c := bA.copy()
    for i := 0; i < len(o.Elems)-1; i++ {
    c.Elems[i] &= ^c.Elems[i]
    }
    i := len(o.Elems) - 1
    if i >= 0 {
    for idx := i * 64; idx < o.Bits; idx++ {
    c.setIndex(idx, c.getIndex(idx) && !o.getIndex(idx))
    }
    }
    return c
    }

this part,
c := bA.copy()
for i := 0; i < len(o.Elems)-1; i++ {
c.Elems[i] &= ^c.Elems[i]
}
I don't understand, is it wrong?

@codecov-io
Copy link

codecov-io commented Sep 28, 2018

Codecov Report

Merging #2506 into develop will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           develop   #2506      +/-   ##
==========================================
+ Coverage    61.35%   61.4%   +0.05%     
==========================================
  Files          197     197              
  Lines        16453   16496      +43     
==========================================
+ Hits         10094   10129      +35     
- Misses        5504    5514      +10     
+ Partials       855     853       -2
Impacted Files Coverage Δ
libs/common/bit_array.go 79.49% <100%> (+0.83%) ⬆️
blockchain/reactor.go 39.44% <0%> (+0.33%) ⬆️
blockchain/pool.go 66.43% <0%> (+0.69%) ⬆️
consensus/reactor.go 74.28% <0%> (+1.16%) ⬆️
libs/db/remotedb/remotedb.go 41.52% <0%> (+4.68%) ⬆️

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

🍰 🌮 🍉

Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

Great catch! The reason this didn't fail before is because our tests never covered this case. I think this function and the associated tests are far too complex, and should be radically simplified in another PR though.

@melekes melekes merged commit c94133e into tendermint:develop Oct 3, 2018
@james-ray james-ray deleted the newBranch1 branch October 3, 2018 11:40
@james-ray
Copy link
Contributor Author

I'm sorry but I found in the CHANGELOG_PENDING.md . The contributor' name is wrong.

  • [common/bit_array] Fixed a bug in the Sub function (@bradyjoestar)

It's not me. Would you please kindly modify it to me? Many thanks.

melekes added a commit that referenced this pull request Oct 10, 2018
@melekes
Copy link
Contributor

melekes commented Oct 10, 2018

Oops, thank you for telling us! Fixed.

@james-ray
Copy link
Contributor Author

I'm really sorry. But would you please modify this line for me too?
Special thanks to external contributors on this release:
@ goolAdapter, @ bradyjoestar

@ValarDragon
Copy link
Contributor

We actually handle that line when building the release! So don't worry, we'll get it :) (Theres several more people missing from that atm)

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

Successfully merging this pull request may close these issues.

None yet

4 participants