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

style: Document recent Go-pointer exceptions #317

Merged

Conversation

wking
Copy link
Contributor

@wking wking commented Jan 26, 2016

The general rule seems to be:

If Go's default value has the same semantics we'd use for an unset
value, don't bother with a pointer.

I'm not sure how well that squares with the pointer motivation,
which sounded like:

We want a consistent way to identify unset settings.

But if the falsy values count as “unset”, maybe the “null is a
consistent identifier for unset” approach was never really viable.

I'm also not sure if the new style extends to integers where zero has
the same semantics as unset values. It sounds like Michael was ok
with no pointers for those values
, but OOMScoreAdj (where zero
clearly means “do nothing”) got a pointer in #233. More clarity
on the threshold would be nice.

@wking wking force-pushed the no-pointers-for-slices-or-maps branch from b28572e to 60b83df Compare January 26, 2016 05:20
@@ -13,9 +13,12 @@ The redundancy reduction from removing the namespacing prefix is not useful enou
## Optional settings should have pointer Go types

So we have a consistent way to identify unset values ([source][optional-pointer]).
The exceptions are slices, maps, and booleans where the default should be false, in which case `omitempty` is sufficient and no pointer is needed ([source][no-pointer-for-slices] and [source][no-pointer-for-boolean]).
Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree with bool values. How can I express "don't change a value of a flag" if the flag was boolean? "false" does not always mean "don't change".
I think it still makes sense to have pointers to booleans.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Tue, Jan 26, 2016 at 10:45:05AM -0800, Vish Kannan wrote:

+The exceptions are slices, maps, and booleans where the default
should be false, in which case omitempty is sufficient and no
pointer is needed ([source][no-pointer-for-slices] and
[source][no-pointer-for-boolean]).

I disagree with bool values. How can I express "don't change a
value of a flag" if the flag was boolean? "false" does not always
mean "don't change".

But sometimes false does mean “don't change” (and those are the
booleans where the default should be false). For example, the source
I link is you asking for a *bool → bool change 1 ;).

If my wording here comes across as “slices, maps, and booleans should
never use pointers” instead of the intended “slices, maps, and
booleans should not use pointers when Go's default for that type means
‘don't do anything’”, then I'm happy to push alternate wording.

Copy link
Contributor

Choose a reason for hiding this comment

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

But sometimes false does mean “don't change” (and those are the
booleans where the default should be false)

That'll be a problem if we want some bool values to be updated (live update), we surely want to change some booleans from true to false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Tue, Jan 26, 2016 at 06:03:44PM -0800, Qiang Huang wrote:

But sometimes false does mean “don't change” (and those are the
booleans where the default should be false)

That'll be a problem if we want some bool values to be updated
(live update), we surely want to change some booleans from true to
false.

Yeah, booleans where ‘false’ requires action should use pointers. I
guess the wording in 60b83df is based on my assumption that “default”
equals “don't do anything” 1. I've pushed 60b83dfd715acf with
wording to explicitly pick out “Go's default for the type is a no-op
in the spec” cases.

The general rule seems to be:

  If Go's default value has the same semantics we'd use for an unset
  value, don't bother with a pointer.

I'm not sure how well that squares with [1]:

  We want a consistent way to identify unset settings.

But if the falsy values count as "unset", maybe the "null is a
consistent identifier for unset" approach was never really viable.

Qiang points out that pointers are required to opt-out of boolean
settings where both true and false would require action [2], so I've
worded the exception to only apply when the Go default for the type is
expicitly a no-op in the spec.

I'm also not sure if the new style extends to integers where zero has
the same semantics as unset values.  It sounds like Michael was ok
with no pointers for those values [3], but OOMScoreAdj (where zero
clearly means "do nothing") got a pointer in opencontainers#233 [4].  More clarity
on the threshold would be nice; in this commit I've laid out the logic
and not explicitly listed the types it applies to.

[1]: opencontainers#233 (comment)
[2]: https://github.com/opencontainers/specs/pull/317/files#r50932706
[3]: opencontainers#233 (comment)
[4]: https://github.com/opencontainers/specs/pull/233/files#diff-34c30be66233f08b447fb608ea0e66bbR206

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking wking force-pushed the no-pointers-for-slices-or-maps branch from 60b83df to d715acf Compare January 27, 2016 05:27
@mrunalp
Copy link
Contributor

mrunalp commented Feb 17, 2016

LGTM

1 similar comment
@crosbymichael
Copy link
Member

LGTM

crosbymichael added a commit that referenced this pull request Feb 17, 2016
style: Document recent Go-pointer exceptions
@crosbymichael crosbymichael merged commit abca05e into opencontainers:master Feb 17, 2016
@wking wking deleted the no-pointers-for-slices-or-maps branch March 3, 2016 21:30
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

5 participants