internal/impl: clean up usePresenceForField() (no-op)
When looking into presence behavior, I noticed that this function
is rather hard to follow. Specifically:
1. usesPresenceArray was only used in the default case,
but re-iterated all conditions of the non-default cases.
usesPresenceArray was a convoluted way to write fd.HasPresence().
2. hasLazyField was a misnomer: the function does not look at entire
messages (which can have a lazy field), but at an individual field.
3. Only messages can be lazy right now, so the definition and usage
of isLazy can be moved into the message case.
4. Instead of the wall-of-text comment above the function,
I have moved the comments to each individual case.
I have verified Google-internally that my new usePresenceForField()
yields exactly the same results as the old one on all our .protos
(see Google-internal CL 753969456 for details).
Change-Id: Ida598fda7f472c71e18802aecabfae3e16de321c
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/670275
Reviewed-by: Lasse Folger <lassefolger@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/internal/impl/message_opaque.go b/internal/impl/message_opaque.go
index dd55e8e..e6b3e41 100644
--- a/internal/impl/message_opaque.go
+++ b/internal/impl/message_opaque.go
@@ -599,29 +599,28 @@
return p.Apply(mi.presenceOffset).PresenceInfo().Present(index)
}
-// usePresenceForField implements the somewhat intricate logic of when
-// the presence bitmap is used for a field. The main logic is that a
-// field that is optional or that can be lazy will use the presence
-// bit, but for proto2, also maps have a presence bit. It also records
-// if the field can ever be lazy, which is true if we have a
-// lazyOffset and the field is a message or a slice of messages. A
-// field that is lazy will always need a presence bit. Oneofs are not
-// lazy and do not use presence, unless they are a synthetic oneof,
-// which is a proto3 optional field. For proto3 optionals, we use the
-// presence and they can also be lazy when applicable (a message).
+// usePresenceForField reports whether the specified field gets an entry in the
+// presence bitmap.
func usePresenceForField(si opaqueStructInfo, fd protoreflect.FieldDescriptor) (usePresence, canBeLazy bool) {
- hasLazyField := fd.(interface{ IsLazy() bool }).IsLazy()
-
- // Non-oneof scalar fields with explicit field presence use the presence array.
- usesPresenceArray := fd.HasPresence() && fd.Message() == nil && (fd.ContainingOneof() == nil || fd.ContainingOneof().IsSynthetic())
switch {
case fd.ContainingOneof() != nil && !fd.ContainingOneof().IsSynthetic():
+ // Oneof fields never use the presence bitmap.
+ //
+ // Synthetic oneofs are an exception: Those are used to implement proto3
+ // optional fields and hence should follow non-oneof field semantics.
return false, false
+
case fd.IsMap():
+ // Map-typed fields never use the presence bitmap.
return false, false
+
case fd.Kind() == protoreflect.MessageKind || fd.Kind() == protoreflect.GroupKind:
- return hasLazyField, hasLazyField
+ // Lazy fields always use the presence bitmap (only messages can be lazy).
+ isLazy := fd.(interface{ IsLazy() bool }).IsLazy()
+ return isLazy, isLazy
+
default:
- return usesPresenceArray || (hasLazyField && fd.HasPresence()), false
+ // If the field has presence, use the presence bitmap.
+ return fd.HasPresence(), false
}
}
