Cleanup digest function to use u64 instead of i64 #1327
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 1 LGTMs obtained, and 0 of 24 files reviewed, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, NativeLink.com Cloud / Remote Cache (Legacy Dockerfile Test), Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (22.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable (waiting on @adam-singer)
There was a problem hiding this comment.
Reviewed 24 of 24 files at r1, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and all files reviewed, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Local / ubuntu-22.04, NativeLink.com Cloud / Remote Cache (Legacy Dockerfile Test), Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable
There was a problem hiding this comment.
Reviewed 11 of 24 files at r1, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and all files reviewed, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, NativeLink.com Cloud / Remote Cache (Legacy Dockerfile Test), Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, and 1 discussions need to be resolved
nativelink-util/src/common.rs line 176 at r1 (raw file):
// This is a placeholder value that can help a user identify // that the conversion failed. -255
nit: seems hacky
There was a problem hiding this comment.
Reviewable status: 1 of 1 LGTMs obtained, and all files reviewed, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, NativeLink.com Cloud / Remote Cache (Legacy Dockerfile Test), asan / ubuntu-22.04, docker-compose-compiles-nativelink (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, and 1 discussions need to be resolved
nativelink-util/src/common.rs line 176 at r1 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
nit: seems hacky
Yeah. I started to do a TryFrom, but it quickly got out of hand.
allada
force-pushed
the
digest-function-cleanup
branch
from
57ea600 to
030b5ad
Compare
allada
force-pushed
the
digest-function-cleanup
branch
from
030b5ad to
14aca6e
Compare
It is weird that we use i64 in the code base for this in the first
place. There is some historical reason for having it this way,
specifically around keeping it in sync with the Digest proto, but once
converted we don't need to do this.
Also implement a more standard format/display function for DigestInfo
using the common '{hash}-{size}' format.
Removes direct access to modify the internals of DigestInfo and requires
member access to go through accessors.
allada
force-pushed
the
digest-function-cleanup
branch
from
14aca6e to
d2abfa3
Compare
There was a problem hiding this comment.
Reviewed 7 of 7 files at r2, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and all files reviewed, and pending CI: windows-2022 / stable
It is weird that we use i64 in the code base for this in the first
place. There is some historical reason for having it this way,
specifically around keeping it in sync with the Digest proto, but once
converted we don't need to do this.
Also implement a more standard format/display function for DigestInfo
using the common '{hash}-{size}' format.
Removes direct access to modify the internals of DigestInfo and requires
member access to go through accessors.
It is weird that we use i64 in the code base for this in the first
place. There is some historical reason for having it this way,
specifically around keeping it in sync with the Digest proto, but once
converted we don't need to do this.
Also implement a more standard format/display function for DigestInfo
using the common '{hash}-{size}' format.
Removes direct access to modify the internals of DigestInfo and requires
member access to go through accessors.
It is weird that we use i64 in the code base for this in the first
place. There is some historical reason for having it this way,
specifically around keeping it in sync with the Digest proto, but once
converted we don't need to do this.
Also implement a more standard format/display function for DigestInfo
using the common '{hash}-{size}' format.
Removes direct access to modify the internals of DigestInfo and requires
member access to go through accessors.
It is weird that we use i64 in the code base for this in the first place. There is some historical reason for having it this way, specifically around keeping it in sync with the Digest proto, but once converted we don't need to do this.
Also implement a more standard format/display function for DigestInfo using the common '{hash}-{size}' format.
Removes direct access to modify the internals of DigestInfo and requires member access to go through accessors.
This change is