Hacker News Reader

Comment by Mawr | Hacker News Reader

  type Optional[T any] struct {
    value *T
  }

Don't use a pointer - it's bad for performance.

  func MakeOptional[T any](value *T) Optional[T] {

Use `New` instead - it's the idiomatic name for a constructor in Go. Drop the `Optional` part - the module name suffices - the caller will see: `opt.New`. Personally I just call the module `optional`, I think it's clearer: `optional.New`.

  func (o* Optional[T]) Unwrap() (T, error) {

1. `Unwrap` reminds me of Rust's .Unwrap, which panics. Seems confusing.

2. There's no error here, the situation is equivalent to a missing key in a map - it's enough to return a bool.

Here's my implementation so far: https://gist.github.com/Mawr-BF2/0a60da26f66b82ee87b98b03336.... Only thing I'm not sure about is whether the JSON serialization methods are necessary.

View on HN

I love it. I will update the library to match some of your suggestions.

I agree with peer comment that we should likely defer to the original type for JSON serialization.

> Use `New` instead - it's the idiomatic name for a constructor in Go.

Where do you get this from? I've tried unsuccessfully to find such naming/design guidelines.

It's noted in the Effective Go guide: https://go.dev/doc/effective_go#package-names

Whether a pointer is bad for performance in this case seems hard to tell a priori. There are also performance advantages if Optional packs nicely.

Agreed on performance being in the air. Tbh, I like the non-pointer option because using a pointer was causing the implementation to feel a bit weird. This cleans up a bit of the handling that was annoying (for example in the old implementation Make(&"something") didn't work because you can't reference a constant without first making a variable.

It is just an assumption on my part, I figure that the cost of pointer chasing is generally going to outweigh any disadvantages. It can also help enable optimizations, for example slices and maps that do not contain pointers do not get scanned by the GC ([1]).

[1]: https://github.com/golang/go/commit/85e7bee19f9f26dfca414b1e...

I fully agree with your approach. The important thing is that the user now has the choice to use a pointer or not: they can always use a pointer to the optional if they want to use pointers.

Pointers to pointers are generally something you try to avoid if possible.

> the user now has the choice to use a pointer or not

But any benefits of the packing are gone whether I make a pointer or not.

And either way, no nested pointers are involved. We’re not taking pointers to options in either case.

For json marshalling, I would defer to the underlying type's marshalling if it's present, and return `null` otherwise.

I think this is appropriate in some cases but not others. For example how does the JSON value distinguish between `Some(null)` and `None`?

IMO using Optional type means the inner value must be not null. And if it's Some(null), it should mean exactly same as None.

distinguishing `Some(null)` and `None` is often considered a feature of Optional ;)

to use a tired example: when getting a value out of a map via some `myMap.get(key)`, you may want to distinguish "not present" = `None` and "present, with value null" = `Some(null)`

the right solution is to just not have nulls in the first place, then there's no problem ;)

(Assuming we are discussing the Option type from rust)

Some(null) is not a valid result.

The whole point of the Option type is to let you know:

a) we got a result: Some(value)

b) there is no result: None

It's trivial to store a null pointer in an Option::Some().

  struct Foo;
  
  fn main() {
      let option_with_null: Option<*const Foo> = Some(std::ptr::null());
  
      dbg!(option_with_null);
      dbg!(option_with_null.is_none());
  }

Output:

  [src/main.rs:6] option_with_null = Some(
      0x0000000000000000,
  )
  [src/main.rs:7] option_with_null.is_none() = false

https://play.rust-lang.org/?version=stable&mode=debug&editio...

As the creator of the function, you should try to adhere to the conventions and not try to break them.

I don't disagree, but:

> Some(null) is not a valid result.

It is a valid result. Null pointer and Option::None are still different things, is all.

> Don't use a pointer - it's bad for performance.

That means you must have Optional[*sync.Mutex]. Then what's the point?

Wondering how a result type would be implemented

Reply on news.ycombinator.com