From ad00dd50bf7f5a3d6d850dd2e46264e677c12fab Mon Sep 17 00:00:00 2001 From: Cole Faust Date: Tue, 21 May 2024 14:20:33 -0700 Subject: [PATCH] Return an Optional[T] from Configurable.Get() Previously, Configurable.Get() copied the value in the property, because it needed to return a pointer in order to indicate whether the property was set or not. Now, it returns a ConfigurableOptional[T], which is the same as the pointer, but it prevents users from altering the pointed-to value, so we don't need to copy it. There are still copies for slice properties, because those are also pointers. In the future we may want to consider making an ImmutableList type to use instead. Bug: 323382414 Test: m nothing --no-skip-soong-tests Change-Id: Ic9ed5ba269d10158e3eac1fea272555c9fa5c0e8 --- Android.bp | 9 ++++++ optional/optional.go | 58 +++++++++++++++++++++++++++++++++++++++ proptools/configurable.go | 47 +++++++++++++++++++++++++++++-- 3 files changed, 111 insertions(+), 3 deletions(-) create mode 100644 optional/optional.go diff --git a/Android.bp b/Android.bp index 4f4fd04..ee0ede0 100644 --- a/Android.bp +++ b/Android.bp @@ -116,6 +116,7 @@ bootstrap_go_package { pkgPath: "github.com/google/blueprint/proptools", deps: [ "blueprint-parser", + "blueprint-optional", ], srcs: [ "proptools/clone.go", @@ -142,6 +143,14 @@ bootstrap_go_package { ], } +bootstrap_go_package { + name: "blueprint-optional", + pkgPath: "github.com/google/blueprint/optional", + srcs: [ + "optional/optional.go", + ], +} + bootstrap_go_package { name: "blueprint-bootstrap", deps: [ diff --git a/optional/optional.go b/optional/optional.go new file mode 100644 index 0000000..0d7ef52 --- /dev/null +++ b/optional/optional.go @@ -0,0 +1,58 @@ +// Copyright 2023 Google Inc. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package optional + +// ShallowOptional is an optional type that can be constructed from a pointer. +// It will not copy the pointer, and its size is the same size as a single pointer. +// It can be used to prevent a downstream consumer from modifying the value through +// the pointer, but is not suitable for an Optional type with stronger value semantics +// like you would expect from C++ or Rust Optionals. +type ShallowOptional[T any] struct { + inner *T +} + +// NewShallowOptional creates a new ShallowOptional from a pointer. +// The pointer will not be copied, the object could be changed by the calling +// code after the optional was created. +func NewShallowOptional[T any](inner *T) ShallowOptional[T] { + return ShallowOptional[T]{inner: inner} +} + +// IsPresent returns true if the optional contains a value +func (o *ShallowOptional[T]) IsPresent() bool { + return o.inner != nil +} + +// IsEmpty returns true if the optional does not have a value +func (o *ShallowOptional[T]) IsEmpty() bool { + return o.inner == nil +} + +// Get() returns the value inside the optional. It panics if IsEmpty() returns true +func (o *ShallowOptional[T]) Get() T { + if o.inner == nil { + panic("tried to get an empty optional") + } + return *o.inner +} + +// GetOrDefault() returns the value inside the optional if IsPresent() returns true, +// or the provided value otherwise. +func (o *ShallowOptional[T]) GetOrDefault(other T) T { + if o.inner == nil { + return other + } + return *o.inner +} diff --git a/proptools/configurable.go b/proptools/configurable.go index 06b39a5..452289d 100644 --- a/proptools/configurable.go +++ b/proptools/configurable.go @@ -19,8 +19,37 @@ import ( "slices" "strconv" "strings" + + "github.com/google/blueprint/optional" ) +// ConfigurableOptional is the same as ShallowOptional, but we use this separate +// name to reserve the ability to switch to an alternative implementation later. +type ConfigurableOptional[T any] struct { + shallowOptional optional.ShallowOptional[T] +} + +// IsPresent returns true if the optional contains a value +func (o *ConfigurableOptional[T]) IsPresent() bool { + return o.shallowOptional.IsPresent() +} + +// IsEmpty returns true if the optional does not have a value +func (o *ConfigurableOptional[T]) IsEmpty() bool { + return o.shallowOptional.IsEmpty() +} + +// Get() returns the value inside the optional. It panics if IsEmpty() returns true +func (o *ConfigurableOptional[T]) Get() T { + return o.shallowOptional.Get() +} + +// GetOrDefault() returns the value inside the optional if IsPresent() returns true, +// or the provided value otherwise. +func (o *ConfigurableOptional[T]) GetOrDefault(other T) T { + return o.shallowOptional.GetOrDefault(other) +} + type ConfigurableElements interface { string | bool | []string } @@ -381,10 +410,9 @@ func NewConfigurable[T ConfigurableElements](conditions []ConfigurableCondition, // Get returns the final value for the configurable property. // A configurable property may be unset, in which case Get will return nil. -func (c *Configurable[T]) Get(evaluator ConfigurableEvaluator) *T { +func (c *Configurable[T]) Get(evaluator ConfigurableEvaluator) ConfigurableOptional[T] { result := c.inner.evaluate(c.propertyName, evaluator) - // Copy the result so that it can't be changed from soong - return copyConfiguredValue(result) + return configuredValuePtrToOptional(result) } // GetOrDefault is the same as Get, but will return the provided default value if the property was unset. @@ -682,6 +710,19 @@ func copyConfiguredValue[T ConfigurableElements](t *T) *T { } } +func configuredValuePtrToOptional[T ConfigurableElements](t *T) ConfigurableOptional[T] { + if t == nil { + return ConfigurableOptional[T]{optional.NewShallowOptional(t)} + } + switch t2 := any(*t).(type) { + case []string: + result := any(slices.Clone(t2)).(T) + return ConfigurableOptional[T]{optional.NewShallowOptional(&result)} + default: + return ConfigurableOptional[T]{optional.NewShallowOptional(t)} + } +} + func copyAndDereferenceConfiguredValue[T ConfigurableElements](t *T) T { switch t2 := any(*t).(type) { case []string: