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/types: add signed/unsigned integer types #27965

Merged
merged 12 commits into from Nov 5, 2017

Conversation

@Profpatsch
Copy link
Member

Profpatsch commented Aug 5, 2017

Add types for signed/unsigned integers, as well as a port type.
Documentation for various types and the module in general is improved as well.

  • tested manually for example values
  • checked the rendering of the manual

Followup to #27239.

@Profpatsch Profpatsch requested a review from shlevy Aug 5, 2017

@mention-bot

This comment has been minimized.

Copy link

mention-bot commented Aug 5, 2017

@Profpatsch, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @nbp and @ericsagnes to be potential reviewers.

lib/types.nix Outdated

/* A network port number. */
port = ints.unsigned16;

This comment has been minimized.

@volth

volth Aug 5, 2017

Contributor

maybe port = ints.between 1 65535 (excluding 0) ?

This comment has been minimized.

@Profpatsch

Profpatsch Aug 5, 2017

Author Member

No, 0 is a valid port that means: “give me an ephemeral (some high) port”

This comment has been minimized.

@volth

volth Aug 5, 2017

Contributor

In the context of service options 0 means a magic value, for example

that would be better types.nullOr types.port or some kind of ADT.

Non-magical port is 1..65535, and many port options (for example iptables-related, nat, etc) have no space for magic. Other might have two magic values (say encoded as 0 and -1 or null).

This comment has been minimized.

@Profpatsch

Profpatsch Aug 6, 2017

Author Member

https://www.grc.com/port_0.htm

"Port Zero" does not officially exist. […]
The designers of the original Berkeley UNIX "Sockets" interface, upon which much of the technology and practice we use today is based, set aside the specification of "port 0" to be used as a sort of "wild card" port. When programming the Sockets interface, the provision of a zero value is generally taken to mean "let the system choose one for me". Programmers who specify "port 0" know that it is an invalid port. They are asking the operating system to pick and assign whatever non-zero port is available and appropriate for their purpose.

In favour of simplicity I’d not overcomplicate the option type.
cc @aszlig

This comment has been minimized.

@Infinisil

Infinisil Oct 21, 2017

Contributor

I wouldn't consider using ints.between 1 65535 instead of unsigned16 an overcomplication. I'm with @volth here, it makes more sense to not include 0

This comment has been minimized.

@grahamc

grahamc Oct 22, 2017

Member

I like it, and can imagine more confused users for why port doesn't accept a valid value, than confused users about why their service on port 0 doesn't work.

lib/types.nix Outdated
signed = int;
signed8 = sign 8 256;
signed16 = sign 16 65536;
signed32 = sign 32 4294967296;

This comment has been minimized.

@volth

volth Aug 5, 2017

Contributor

I am for int, int8, int16, int32.
signed is needless synonym

This comment has been minimized.

@Profpatsch

Profpatsch Aug 5, 2017

Author Member

I deliberately decided against int/uint, because

  • ints.int8 is redundant
  • ints.signed8 is more specific
  • more importantly, it would lead to uint, which is just a bad abbreviation

This comment has been minimized.

@volth

volth Aug 5, 2017

Contributor

unsigned or unsigned-int is not bad, but signed seems excessive when there is int already (well, there is signed in C, but nobody uses it or remember that there is such alias for int)

This comment has been minimized.

@Profpatsch

Profpatsch Aug 5, 2017

Author Member

So un-signed is intuitive while nobody knows what signed means?

This comment has been minimized.

@volth

volth Aug 5, 2017

Contributor

Kind of. uint might be a bad abbreviation, but of those who type uint quite a few recall what it abbreviates u[n-signed ]int. Just a U-word no matter what is after U. S-word is less known.
For example u8 at type position is readable, i8 too, but what about s8?

This comment has been minimized.

@Profpatsch

Profpatsch Aug 6, 2017

Author Member

I don’t buy your reasoning. If one didn’t know what signed means, one shouldn’t know what unsigned means as well. If one knew what unsigned means, the step to signed should be trivial. uint is the worst abbreviation of all. u8 and i8 are maybe easy for Rust programmers, but horrible abbreviations out of low-level system programming context.

This comment has been minimized.

@volth

This comment has been minimized.

@Profpatsch

Profpatsch Aug 6, 2017

Author Member

I won’t discuss this any further, since it’s too bike-sheddy for my taste.

This comment has been minimized.

@volth

volth Aug 6, 2017

Contributor

ok, but not only.
My first (and still) point is not about which word to prefer, but against using two words for the same entity: against signed = int;. All the rest are suggesting ways on how to avoid it, which is opinionated and bike-sheddy. I do not mind signed if it would deprecate int.

@grahamc

This comment has been minimized.

Copy link
Member

grahamc commented Oct 22, 2017

@Profpatsch can you rebase? This PR seems to be based on a fairly old master.

lib/types.nix Outdated
name = "intBetween";
description = "Integer between ${betweenDesc lowest highest}.";
};
ign = lowest: highest: name: docStart:

This comment has been minimized.

@grahamc

grahamc Oct 22, 2017

Member

what does ign mean?

This comment has been minimized.

@Profpatsch

Profpatsch Oct 23, 2017

Author Member

Helper w/ cute name only used internally in sign and unsign directly below.

lib/types.nix Outdated

intBetween = min: max:

This comment has been minimized.

@grahamc

grahamc Oct 22, 2017

Member

are you intentionally removing this from the API? Perhaps a deprecation notice and map it back to int.between?

This comment has been minimized.

@grahamc

grahamc Oct 22, 2017

Member

Oh, it seems you put it back.

respectively (e.g. <literal>−128</literal> to <literal>127</literal>
for 8 bits).
</para></listitem>
</varlistentry>

This comment has been minimized.

@grahamc

grahamc Oct 22, 2017

Member

Would you mind breaking these out, and specifying exactly their min/max?

This comment has been minimized.

@Profpatsch

Profpatsch Oct 31, 2017

Author Member

Not sure if that’d improve the description, since those are based on byte-lengths anyway.

nixos/doc/manual/development/option-types.xml Outdated
<listitem><para>An integer between <replaceable>min</replaceable>
and <replaceable>max</replaceable> (both inclusive).
Useful for e.g. port ranges.
<listitem><para>An unsigned integer (that is ≥ 0).

This comment has been minimized.

@grahamc

grahamc Oct 22, 2017

Member

Hmm... I'm not sure we should put unicode here, given these docs are sometimes read on TTY8.

This comment has been minimized.

@Profpatsch

Profpatsch Oct 30, 2017

Author Member

Doesn’t seem to be a problem for these:
screenshot

Do you refer to the installation-manual tty? Is that not in UTF-8?

This comment has been minimized.

@Profpatsch

Profpatsch Nov 3, 2017

Author Member

Removed it, since it’s mandatory that it shows up everywhere.

@Profpatsch Profpatsch requested a review from edolstra as a code owner Oct 31, 2017

@Profpatsch Profpatsch force-pushed the Profpatsch:intBetween branch Oct 31, 2017

Profpatsch added some commits Jul 8, 2017

lib/types: add intBetween
An int type that checks the value range.
lib/modules: Change type error to be gramatically nicer
Before:
  <x> is not a integer between 0 and 100 (inclusively).
(notice that “a” is wrong, it should be “an”)
Now:
  <x> is not of type `integer between 0 and 100 (inclusively)'.

This sounds a bit more formal, but circumvents the grammatical problems.
Multi-word type descriptions are also easier to see.
lib/types: add various signed/unsigned int types
It is sometimes necessary to restrict the domain of integers for configurations,
as well as restricting them to unsigned/positive values.

@Profpatsch Profpatsch force-pushed the Profpatsch:intBetween branch Oct 31, 2017

@Profpatsch Profpatsch requested a review from nbp Oct 31, 2017

@nbp
Copy link
Member

nbp left a comment

This set of patches moves things around, adding section names, and does not only add int types.

Please restrict this PR to adding int types.

@@ -338,7 +338,7 @@ rec {
# Type-check the remaining definitions, and merge them.
mergedValue = foldl' (res: def:
if type.check def.value then res
else throw "The option value `${showOption loc}' in `${def.file}' is not a ${type.description}.")
else throw "The option value `${showOption loc}' in `${def.file}' is not of type `${type.description}'.")

This comment has been minimized.

@nbp

nbp Nov 2, 2017

Member

From what I understand we are moving away from asymmetrical quoting?
This set of commit changes the quoting within comments as well, please revert to do only one type of quotation mark.

This comment has been minimized.

@Profpatsch

Profpatsch Nov 3, 2017

Author Member

We are? My impression was Eelco would like to keep the “special” quoting?

This comment has been minimized.

@Profpatsch

Profpatsch Nov 3, 2017

Author Member
12:03:46        niksnut | using `...' is always wrong

heh. :)

This comment has been minimized.

@zimbatm

zimbatm Nov 3, 2017

Member

Unicode quotes have practical implications that make them undesirable.

See NixOS/rfcs#4

This comment has been minimized.

@Profpatsch

Profpatsch Nov 3, 2017

Author Member

` is not unicode, though.

lib/types.nix Outdated
@@ -107,22 +107,80 @@ rec {
merge = mergeEqualOption;
};

### INTEGERS

This comment has been minimized.

@nbp

nbp Nov 2, 2017

Member

nit: ### Integers , do the same below for other comments. Indent this comment at the same level as the code.

lib/types.nix Outdated
merge = mergeOneOption;
};

# specialized subdomains of int

This comment has been minimized.

@nbp

nbp Nov 2, 2017

Member

nit: # Specialized ...

lib/types.nix Outdated
@@ -196,6 +254,15 @@ rec {
functor = (defaultFunctor name) // { wrapped = elemType; };
};

# TODO: deprecate, like `list'

This comment has been minimized.

@nbp

nbp Nov 2, 2017

Member

attrs is not deprecated! Remove this miss-leading comment.

This comment has been minimized.

@Profpatsch

Profpatsch Nov 3, 2017

Author Member

In the removed doc commit. I hope I don’t forget to remove it when PR-ing that.

lib/types.nix Outdated
# Merge multiple definitions by concatenating them (with the given
# separator between the values).
/* Merge multiple definitions by concatenating them (with the given
separator between the values). */

This comment has been minimized.

@nbp

nbp Nov 2, 2017

Member

nit: Avoid /* */ comments for small descriptions.

This comment has been minimized.

@Profpatsch

Profpatsch Nov 3, 2017

Author Member

I’d like to make /**/ default for “docstrings”, so that they can be distinguished from in-line comments easily.

Profpatsch added some commits Aug 5, 2017

lib/types: nixos manual documentation for signed/unsinged int
Synchronize the manual for the new types.
lib/types: remove port type again
Will be introduced as a taggedUnion, once that type is in nixpkgs.
lib/types: signed -> s, unsigned -> u, remove signed alias
Mirrors the way it’s done in modern low-level languages like Rust (by input of
@nbp).
Removes the signed alias for int.
lib/types: add tests for `ints.between` and `ints.unsigned`
The int types are trivial invocations of `ints.between`, so they are not tested
explicitely.
lib/types: add `ints.positive`.
For values that are positive, but cannot be 0.

@Profpatsch Profpatsch force-pushed the Profpatsch:intBetween branch to 4bc58bd Nov 3, 2017

@Profpatsch

This comment has been minimized.

Copy link
Member Author

Profpatsch commented Nov 3, 2017

@nbp I removed the documentation commit that shuffles part of the file around and addressed your remarks.

@nbp

nbp approved these changes Nov 4, 2017

Copy link
Member

nbp left a comment

nit: There is still a quoting issue in the newly added code.
nit: Changing the quotes of the existing code in unrelated to adding integer types, and as such belong to a different patch.

@Profpatsch

This comment has been minimized.

Copy link
Member Author

Profpatsch commented Nov 5, 2017

Will make an additional PR for the remaining things with wrong quoting style afterwards. It’s mainly in lib/modules.nix.

@Profpatsch Profpatsch merged commit 213bd28 into NixOS:master Nov 5, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
grahamcofborg-eval Evaluation checks OK
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.