Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3808 +/- ##
==========================================
+ Coverage 25.48% 25.89% +0.41%
==========================================
Files 668 673 +5
Lines 42848 43206 +358
==========================================
+ Hits 10918 11189 +271
- Misses 30929 30997 +68
- Partials 1001 1020 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cthulhu-rider
requested review from
End-rey,
carpawell and
roman-khimov
as code owners
| var prevNum protowire.Number | ||
| loop: | ||
| for { | ||
| num, typ, n, err := iprotobuf.ParseTag(buf[off:]) |
There was a problem hiding this comment.
If len(buf)==0, here we have an error? Maybe we can do a check at the beginning of the function?
There was a problem hiding this comment.
i dont expect this to be called with empty buffer. If so, this is an error to me
There was a problem hiding this comment.
This depends on use, if used by FSTree directly it can be problematic, empty buffer can appear as a result of a file read (touch fstree/a/b/c/dobj), erroring out is better than panic in this case.
cthulhu-rider
force-pushed
the
protolib
branch
2 times, most recently
from
ac5c79e to
525df92
Compare
| fieldObjectHeader | ||
| fieldObjectPayload | ||
|
|
||
| FieldHeaderVersion = 1 |
There was a problem hiding this comment.
Is there any way to avoid these redefinitions? Like some variable initialization in init() via https://pkg.go.dev/reflect#StructTag (SDK proto packages have this data) or some autogenerator?
There was a problem hiding this comment.
they are declared in SDK, we can expose them. Codegen does not have them. Reflection is an overkill to me. So, const declaration is the easiest way to me now
There was a problem hiding this comment.
Certainly we don't want another copy of this set. Then ideally it should be autogenerated in SDK as well, but that's a different story.
| var prevNum protowire.Number | ||
| loop: | ||
| for { | ||
| num, typ, n, err := iprotobuf.ParseTag(buf[off:]) |
There was a problem hiding this comment.
This depends on use, if used by FSTree directly it can be problematic, empty buffer can appear as a result of a file read (touch fstree/a/b/c/dobj), erroring out is better than panic in this case.
internal/object/wire.go
Outdated
| case FieldHeaderSplitParent: | ||
| idf, err = iprotobuf.ParseLENFieldBounds(buf, off, n, num, typ) | ||
| if err != nil { | ||
| return idf, sigf, hdrf, err |
There was a problem hiding this comment.
Parse first, switch/assign later, deduplicate this thing.
There was a problem hiding this comment.
not all cases do the same. I prefer to leave as is
There was a problem hiding this comment.
Parsing is all the same, that's just pure code duplication. Then switch/case.
| return 0, 0, 0, err | ||
| } | ||
|
|
||
| if len(buf) == 0 { |
There was a problem hiding this comment.
BTW, this is checked, unlike GetNonPayloadFieldBounds.
| TagBytes3 = 26 | ||
| TagBytes4 = 34 | ||
| TagBytes5 = 42 | ||
| TagBytes6 = 50 |
There was a problem hiding this comment.
Don't proto libraries have it in some form?
There was a problem hiding this comment.
they could be var _ = protowire.EncodeTag(n, protowire.BytesType), but they are constant essentially, i dont wanna have exported variables when possible
There was a problem hiding this comment.
Would be nice to autogen/autofill, but at the same time this isn't likely to change in any way. Maybe at least mention how it's created in comment or test it against EncodeTag.
Provide functionality which is going to be used for convenient protobuf parsing. In particular, for #3783. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
cthulhu-rider
force-pushed
the
protolib
branch
from
525df92 to
e0fa335
Compare
Allows to work with object binary w/o full unmarshalling. Going to be used in #3783. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
cthulhu-rider
force-pushed
the
protolib
branch
from
e0fa335 to
1935f0b
Compare
It turned out that field numbers are needed for flexible protobuf'ing, as in nspcc-dev/neofs-node#3808. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
It turned out that field numbers are needed for flexible protobuf'ing, as in nspcc-dev/neofs-node#3808. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
It turned out that field numbers are needed for flexible protobuf'ing, as in nspcc-dev/neofs-node#3808. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
It turned out that field numbers are needed for flexible protobuf'ing, as in nspcc-dev/neofs-node#3808. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
3 participants
extracted from #3792