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

lib/bit-representation: init #258834

Closed
wants to merge 1 commit into from
Closed

Conversation

lucasew
Copy link
Contributor

@lucasew lucasew commented Oct 3, 2023

Description of changes

Add lib functions to parse and stringify integers following a base such as binary and hexadecimal.

Blocks:

  • Implementation of hash based proof of concept for [RFC 0151] NixOS port allocator rfcs#151
    • Actually, for the proof of concept I did a workaround using builtins.fromTOML. Final implementation might have to be different.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Oct 3, 2023
@lucasew lucasew requested a review from kevincox October 3, 2023 17:01
@lucasew lucasew changed the title lib/bitrepr: init lib/bit-representation: init Oct 3, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Oct 3, 2023
@lucasew
Copy link
Contributor Author

lucasew commented Oct 4, 2023

I found a bug in the conversion from int to string. I also need to realize how to do tests for the lib.

@lucasew
Copy link
Contributor Author

lucasew commented Oct 4, 2023

Bug solved. The remainders where being generated reversed. I just had to reverse that list and everything is now working nice.

@lucasew
Copy link
Contributor Author

lucasew commented Oct 4, 2023

I also added tests, some of them compared with results from Python.

@lucasew lucasew marked this pull request as ready for review October 4, 2023 17:22
@lucasew
Copy link
Contributor Author

lucasew commented Oct 4, 2023

Result of nixpkgs-review pr 258834 run on x86_64-linux 1

1 package blacklisted:
  • nixos-install-tools

lib/default.nix Outdated Show resolved Hide resolved
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

These char -> int attribute sets should not be exposed in the interface, it's an implementation detail.

There's also already lib.toHexString and lib.toBaseDigits which this PR should unify with.

I also think it should be called lib.intRepresentations or so, "bit" would only match for binary.

@lucasew
Copy link
Contributor Author

lucasew commented Oct 7, 2023

These char -> int attribute sets should not be exposed in the interface, it's an implementation detail.

There's also already lib.toHexString and lib.toBaseDigits which this PR should unify with.

I also think it should be called lib.intRepresentations or so, "bit" would only match for binary.

Whoa, I didn't know about toBaseDigits. It's almost the same thing I am trying to do here lol.

@lucasew lucasew force-pushed the lib/bitrepr branch 2 times, most recently from bfcccc5 to 7938713 Compare October 7, 2023 14:35
@lucasew lucasew requested a review from infinisil October 7, 2023 14:35
@lucasew
Copy link
Contributor Author

lucasew commented Oct 7, 2023

These char -> int attribute sets should not be exposed in the interface, it's an implementation detail.

There's also already lib.toHexString and lib.toBaseDigits which this PR should unify with.

I also think it should be called lib.intRepresentations or so, "bit" would only match for binary.

I changed the implementation of toHexString to use intRepresentations and added the possibility for one to only pass the name of the base instead of the mapping.

I also refactored the mapping logic to be a list and when parsing it also builds a lookup table that makes that attrset so lookups are faster and it's harder to provide invalid inputs.

I am not sure if I should keep that lookup table function there or move somewhere else tho.

@infinisil
Copy link
Member

Hmm still not very happy with the interface, how about something like

  • formatBinary :: Int -> String: Formats a number as binary
  • formatOctal :: Int -> String: Formats a number as octal
  • formatHex :: Int -> String: Formats a number as hexadecimal

Also parseBinary, parseOctal, etc.

But I guess much more importantly, can you show why we need all of these? We shouldn't add functions without use cases to lib.

@lucasew
Copy link
Contributor Author

lucasew commented Oct 9, 2023

Hmm still not very happy with the interface, how about something like

  • formatBinary :: Int -> String: Formats a number as binary
  • formatOctal :: Int -> String: Formats a number as octal
  • formatHex :: Int -> String: Formats a number as hexadecimal

Also parseBinary, parseOctal, etc.

But I guess much more importantly, can you show why we need all of these? We shouldn't add functions without use cases to lib.

Initially, parsing a hex hash to decompose in values to use with the port allocator RFC implementation. That way we can generate valid ports that are less likely to have collisions.

@infinisil
Copy link
Member

That hinges on the RFC/PR being accepted/merged, and from what I could tell it's not close to that, so I don't think we can use that as justification. And this would only be a justification for parseHex, not any of the other functions.

@AndersonTorres
Copy link
Member

That hinges on the RFC/PR being accepted/merged, and from what I could tell it's not close to that

Then let's keep it as a proof-of-concept.

And this would only be a justification for parseHex, not any of the other functions.

Completeness.

lib/int-representations.nix Outdated Show resolved Hide resolved
=> {a = 0; b = 1; c = 2; }
*/
mkLookupTable =
# List of symbols of the base alphabet. Base number is inferred from the size. Can also be the name of the base as string such as 'bin' or 'hex'.
Copy link
Member

Choose a reason for hiding this comment

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

Too long. 80-column limit, please

lib/tests/misc.nix Outdated Show resolved Hide resolved
@infinisil
Copy link
Member

Completeness.

I'm very much against merging 5 functions without any use cases just because of "completeness". Interfaces without any use cases are but a maintenance burden. We can always easily add more functions later if the use cases do happen to arise.

@lucasew
Copy link
Contributor Author

lucasew commented Oct 10, 2023

Completeness.

I'm very much against merging 5 functions without any use cases just because of "completeness". Interfaces without any use cases are but a maintenance burden. We can always easily add more functions later if the use cases do happen to arise.

At first I only wanted to add a hex parser to int. That's what I need actually, but as the logic keeps the same I added support for arbitrary mappings. BTW maybe it's best to remove the case normalization (all uppercase for example) and that mapping aliases and then add a fromHEXString to follow the same but inverse signature as toHEXString.

@lucasew lucasew force-pushed the lib/bitrepr branch 2 times, most recently from 354e970 to 9c173c8 Compare October 10, 2023 19:08
Signed-off-by: lucasew <lucas59356@gmail.com>
@lucasew
Copy link
Contributor Author

lucasew commented Nov 10, 2023

Did a rebase

@lucasew lucasew closed this Nov 10, 2023
@lucasew lucasew mentioned this pull request Nov 10, 2023
13 tasks
@Janik-Haag
Copy link
Member

But I guess much more importantly, can you show why we need all of these? We shouldn't add functions without use cases to lib.

When working with ip addresses and subnets having functions to convert to and from binary and hex to decimal is incredibly useful. Like writing a function to check if a ip is part of a subnet basically requires converting it and it's netmask to binary and throwing and logic on it, or when doing any calculations with ipv6 where the entire ip is represented as hex values.

@infinisil
Copy link
Member

@Janik-Haag Sounds like a good use case to me!

@lucasew lucasew deleted the lib/bitrepr branch May 23, 2024 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants