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

Replace bitset with std math/big #159

Merged
merged 1 commit into from
Sep 23, 2021
Merged

Conversation

zhsj
Copy link
Contributor

@zhsj zhsj commented Sep 20, 2021

No description provided.

@rhatdan
Copy link
Collaborator

rhatdan commented Sep 20, 2021

@kolyshkin @thaJeztah PTAL

Comment on lines 647 to 648
low: &level{cats: new(big.Int)},
high: &level{cats: new(big.Int)},
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK there's no need to explicitly initialize these.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, those are pointers... Anyway, it's better initialize when needed (see below).

}

Copy link
Collaborator

Choose a reason for hiding this comment

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

please keep this empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Collaborator

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

Nice work, LGTM except a nit about initialization

@zhsj zhsj force-pushed the bitset branch 2 times, most recently from 71d72f5 to 8e14aa1 Compare September 21, 2021 03:36
@thaJeztah
Copy link
Member

looks like this was updated; @kolyshkin ptal if your comments were addressed 🤗

if str != "" {
str += ","
}
str += "c" + strconv.Itoa(int(i))
str += "c" + strconv.FormatInt(int64(i), 10)
Copy link
Collaborator

Choose a reason for hiding this comment

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

From the docs:

Itoa is equivalent to FormatInt(int64(i), 10).

so why change this?

Copy link
Contributor Author

@zhsj zhsj Sep 22, 2021

Choose a reason for hiding this comment

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

no particular reason, probably i deleted the old lines when rewrote and used my own tastes.

grep the strconv usage in this package, seems itoa and atoi are more used. let's keep the style.

Comment on lines 574 to 576
str += ",c" + strconv.FormatInt(int64(i), 10)
} else if length > 1 {
str += ".c" + strconv.FormatInt(int64(i), 10)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, strconv.Itoa(i) seems easier (feel free to ignore though).

kolyshkin
kolyshkin previously approved these changes Sep 22, 2021
Copy link
Collaborator

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@gitstashpop PTAL

@kolyshkin
Copy link
Collaborator

LGTM, but would like @gitstashpop to take a look before merging this.

kolyshkin
kolyshkin previously approved these changes Sep 22, 2021
Copy link
Collaborator

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

gitstashpop
gitstashpop previously approved these changes Sep 22, 2021
Copy link
Contributor

@gitstashpop gitstashpop left a comment

Choose a reason for hiding this comment

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

changes lgtm, just a nit about for loop style


i := int(c.TrailingZeroBits())
length := 0
for i < c.BitLen() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this could be a for init; condition; increment loop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: Shengjing Zhu <i@zhsj.me>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

@kolyshkin @gitstashpop ptal (change was pushed and made your last review "outdated")

Copy link
Contributor

@gitstashpop gitstashpop left a comment

Choose a reason for hiding this comment

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

lgtm

@rhatdan rhatdan merged commit 63d6f5a into opencontainers:main Sep 23, 2021
@rhatdan
Copy link
Collaborator

rhatdan commented Sep 23, 2021

Thanks @zhsj

@zhsj zhsj deleted the bitset branch September 23, 2021 14:48
This was referenced Oct 5, 2021
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.

5 participants