From 93bccf763ed3bb7b73829a600f447b078d99af1c Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Mon, 10 Feb 2020 11:14:03 -0800 Subject: [PATCH] all: scrub all TODOs TODOs that we do not intend to address have been deleted. Those that are blocking v2 release are marked with "blocks". Change-Id: I7efa9e546d0637b562101d0edc7009893d762722 Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/218878 Reviewed-by: Damien Neil --- internal/errors/is_go112.go | 3 --- internal/impl/message_reflect.go | 2 +- proto/reset.go | 2 +- reflect/protodesc/desc.go | 2 +- reflect/protoreflect/type.go | 5 ----- reflect/protoreflect/value.go | 10 ++-------- reflect/protoregistry/registry.go | 23 +---------------------- testing/prototest/prototest.go | 3 +++ 8 files changed, 9 insertions(+), 41 deletions(-) diff --git a/internal/errors/is_go112.go b/internal/errors/is_go112.go index 0f5e5850..f90e909b 100644 --- a/internal/errors/is_go112.go +++ b/internal/errors/is_go112.go @@ -22,9 +22,6 @@ func Is(err, target error) bool { if x, ok := err.(interface{ Is(error) bool }); ok && x.Is(target) { return true } - // TODO: consider supporing target.Is(err). This would allow - // user-definable predicates, but also may allow for coping with sloppy - // APIs, thereby making it easier to get away with them. if err = unwrap(err); err == nil { return false } diff --git a/internal/impl/message_reflect.go b/internal/impl/message_reflect.go index f5f7f2bf..cff90672 100644 --- a/internal/impl/message_reflect.go +++ b/internal/impl/message_reflect.go @@ -330,7 +330,7 @@ func (mi *MessageInfo) checkField(fd pref.FieldDescriptor) (*fieldInfo, pref.Ext if fd.IsExtension() { if fd.ContainingMessage().FullName() != mi.Desc.FullName() { - // TODO: Should this be exact containing message descriptor match? + // TODO(blocks): Should this be exact containing message descriptor match? panic("mismatching containing message") } if !mi.Desc.ExtensionRanges().Has(fd.Number()) { diff --git a/proto/reset.go b/proto/reset.go index dcf70b91..b861f5bf 100644 --- a/proto/reset.go +++ b/proto/reset.go @@ -8,7 +8,7 @@ import "google.golang.org/protobuf/reflect/protoreflect" // Reset clears every field in the message. func Reset(m Message) { - // TODO: Document memory aliasing guarantees. + // TODO(blocks): Document memory aliasing guarantees. if mr, ok := m.(interface{ Reset() }); ok && hasProtoMethods { mr.Reset() return diff --git a/reflect/protodesc/desc.go b/reflect/protodesc/desc.go index 84f29196..f40a0171 100644 --- a/reflect/protodesc/desc.go +++ b/reflect/protodesc/desc.go @@ -67,7 +67,7 @@ func allowUnresolvable() option { // the path must be unique. The newly created file descriptor is not registered // back into the provided file registry. func NewFile(fd *descriptorpb.FileDescriptorProto, r Resolver) (protoreflect.FileDescriptor, error) { - // TODO: remove setting allowUnresolvable once naughty users are migrated. + // TODO(blocks): remove setting allowUnresolvable once naughty users are migrated. return newFile(fd, r, allowUnresolvable()) } func newFile(fd *descriptorpb.FileDescriptorProto, r Resolver, opts ...option) (protoreflect.FileDescriptor, error) { diff --git a/reflect/protoreflect/type.go b/reflect/protoreflect/type.go index 31c7d82a..91380e59 100644 --- a/reflect/protoreflect/type.go +++ b/reflect/protoreflect/type.go @@ -451,11 +451,6 @@ type ExtensionType interface { // TypeDescriptor returns the extension type descriptor. TypeDescriptor() ExtensionTypeDescriptor - // TODO: What to do with nil? - // Should ValueOf(nil) return Value{}? - // Should InterfaceOf(Value{}) return nil? - // Similarly, should the Value.{Message,List,Map} also return nil? - // ValueOf wraps the input and returns it as a Value. // ValueOf panics if the input value is invalid or not the appropriate type. // diff --git a/reflect/protoreflect/value.go b/reflect/protoreflect/value.go index c7da263c..5ea914de 100644 --- a/reflect/protoreflect/value.go +++ b/reflect/protoreflect/value.go @@ -90,8 +90,6 @@ type Message interface { // of the value; to obtain a mutable reference, use Mutable. Get(FieldDescriptor) Value - // TODO: Should Set of a empty, read-only value be equivalent to Clear? - // Set stores the value for a field. // // For a field belonging to a oneof, it implicitly clears any other field @@ -157,8 +155,6 @@ type Message interface { // "google.golang.org/protobuf/runtime/protoiface".Methods. // Consult the protoiface package documentation for details. ProtoMethods() *methods - - // TODO: Add method to retrieve ExtensionType by FieldNumber? } // RawFields is the raw bytes for an ordered sequence of fields. @@ -204,9 +200,7 @@ type List interface { // Append is a mutating operation and unsafe for concurrent use. Append(Value) - // TODO: Should there be a Mutable and MutableAppend method? - - // TODO: Should truncate accept two indexes similar to slicing? + // TODO(blocks): Should there be a AppendMutable method? // Truncate truncates the list to a smaller length. // @@ -264,7 +258,7 @@ type Map interface { // Set is a mutating operation and unsafe for concurrent use. Set(MapKey, Value) - // TODO: Should there be a Mutable method? + // TODO(blocks): Should there be a Mutable method? // NewValue returns a new value assignable as a map value. // For enums, this returns the first enum value. diff --git a/reflect/protoregistry/registry.go b/reflect/protoregistry/registry.go index 8385c751..322ce374 100644 --- a/reflect/protoregistry/registry.go +++ b/reflect/protoregistry/registry.go @@ -32,7 +32,7 @@ var ignoreConflict = func(d protoreflect.Descriptor, err error) bool { log.Printf(""+ "WARNING: %v\n"+ "A future release will panic on registration conflicts.\n"+ - // TODO: Add a URL pointing to documentation on how to resolve conflicts. + // TODO(blocks): Add a URL pointing to documentation on how to resolve conflicts. "\n", err) return true } @@ -396,27 +396,6 @@ var ( // Types is a registry for looking up or iterating over descriptor types. // The Find and Range methods are safe for concurrent use. type Types struct { - // TODO: The syntax of the URL is ill-defined and the protobuf team recently - // changed the documented semantics in a way that breaks prior usages. - // I do not believe they can do this and need to sync up with the - // protobuf team again to hash out what the proper syntax of the URL is. - - // TODO: Should we separate this out as a registry for each type? - // - // In Java, the extension and message registry are distinct classes. - // Their extension registry has knowledge of distinct Java types, - // while their message registry only contains descriptor information. - // - // In Go, we have always registered messages, enums, and extensions. - // Messages and extensions are registered with Go information, while enums - // are only registered with descriptor information. We cannot drop Go type - // information for messages otherwise we would be unable to implement - // portions of the v1 API such as ptypes.DynamicAny. - // - // There is no enum registry in Java. In v1, we used the enum registry - // because enum types provided no reflective methods. The addition of - // ProtoReflect removes that need. - typesByName typesByName extensionsByMessage extensionsByMessage diff --git a/testing/prototest/prototest.go b/testing/prototest/prototest.go index 7a403a90..050a61dd 100644 --- a/testing/prototest/prototest.go +++ b/testing/prototest/prototest.go @@ -37,6 +37,9 @@ type MessageOptions struct { } } +// TODO(blocks): TestMessage should not take in MessageOptions, +// but have a MessageOptions.Test method instead. + // TestMessage runs the provided m through a series of tests // exercising the protobuf reflection API. func TestMessage(t testing.TB, m proto.Message, opts MessageOptions) {