Struct fields numeric labels #160

gobwas · 2016-07-15T12:06:48Z

Hello!

This pull request is about this issue.

I think it could be useful. Some integration protocols use scheme, when map keys are numerical (int or uint), for example, to reduce network usage and other performance needs.

Regards,
Sergey.

This change is

gobwas · 2016-07-15T12:20:37Z

Also, I have a question about decision according to this thread.

If it is necessary to declare label types also as int32, int64, uint8 etc., that is, strong typed labels without dependency on a value, I could implement it.

philhofer · 2016-07-15T18:31:32Z

Hey, thanks for the PR.

This is going to take me a day or two to review, since it's a substantial change.

Just for my own edification, could you point me to a place where this would be used 'in the wild?'

philhofer · 2016-07-15T18:36:10Z

Also, can I suggest the following for syntax:

type Foo struct {
    Bar string `msg:0x03"
}

We should be able to disambiguate string field names from integer field names simply by the presence of quotes. I'd also make unsigned integers the default, and then make int(0x03) mean signed integers, since I'm imagining most use-cases will only use unsigned integers...?

philhofer · 2016-07-15T18:38:36Z

gen/spec.go

+		case string:
+			t = msgp.StrType
+		default:
+			panic(fmt.Errorf("field %q has unknown tag type %T", f.FieldName, f.FieldTag))


Don't panic here; print an error and exit. Same goes for field label parsing.

There are already few cases when generator panics. For example, here. So is it really necessary to print error and do some of os.Exit(1)?

gobwas · 2016-07-15T19:12:10Z

Hey @philhofer,

Yeah, I agree with syntax you suggested.

But my question above is still actual – should we take care on strong typed labels, like uint8?
For example, we could use uint by default, and then add ability to cast not only to signed int, but to any other type of int? How do you think?

About usage 'in the wild' I do not clearly understand your question. If this is about concrete use case – I need this to implement some binary protocol to integrate with. It is very much like, for example, this one.

philhofer · 2016-07-15T20:49:44Z

I need this to implement some binary protocol to integrate with. It is very much like, for example, this one.

Great; I was just curious about who was serializing things this way already.

But my question above is still actual – should we take care on strong typed labels, like uint8?
For example, we could use uint by default, and then add ability to cast not only to signed int, but to any other type of int? How do you think?

Practically speaking, I'd imagine most things will end up being encoded as a 127-bit fixint anyway (and most everything else in one of the fixed-width unsigned types), so I don't think this is necessary, but I'm happy to be convinced otherwise.

gobwas · 2016-07-22T09:18:01Z

Hello @philhofer!

Just returned to complete this feature. Please look what I have found about the use of field tag values without quotes: play.golang.org. That is, it is not possible to distinguish numeric label vs. string simply by presence of quoutes without breaking other tags cause it is violates "conventional format".
So I could suggest to use something like this (I decided that (uint)0x01 syntax is bad because of super low, but probability of shadowing user key):

type Foo struct {
    Bar string `msg:"0x03,uint"`
}

gobwas · 2016-07-23T08:54:12Z

gen/spec.go

+		case string:
+			t = msgp.StrType
+		default:
+			panic(fmt.Sprintf(


Will duplicate my previous comment to not loose it:

There are already few cases when generator panics. For example, here. So is it really necessary to print error and do some of os.Exit(1)?

If it is, I could move logging logic from parse package to separate package, e.g. internal/log, and reuse functions like warnf,warnln etc., and also refactor fatalf to print log message and do os.Exit(1).

That sounds great.

gobwas · 2016-07-29T12:22:47Z

@philhofer ping 👀

philhofer · 2016-08-02T00:08:23Z

parse/getast.go

+			var err error
+			switch tags[1] {
+			case "uint":
+				str, base := numeric(tags[0])


strconv.ParseInt(tags[0], 0, 64) will infer hex or octal based on the prefix; no need to do that yourself.

gobwas · 2016-08-02T13:58:13Z

@philhofer thanks! Applied your comments, and moved logging logic to the internal/log package. I did not changed already written panic's to log.Fatal (except panic that I have wrote).

gobwas · 2016-08-08T10:35:13Z

Hello @philhofer! Is it able to merge this? ) It becomes little bit uncomfortable to live with fork 😞

gobwas · 2016-09-14T13:13:02Z

Ping @philhofer =)

gobwas · 2016-09-14T15:02:48Z

I have also refactored code generation for maps – now you could marshal/unmarshal not only map[string]T.

gobwas · 2016-10-17T17:36:35Z

@philhofer could you stop ignoring this? =)

pwaller · 2016-11-11T17:04:57Z

FYI @gobwas, the tests failed (I'm not associated with this project, but just thought you might like to know):

--- FAIL: TestReadIntf (0.00s)
    read_test.go:63: map[thing-1:thing-1-value thing-2:800 thing-3:[115 111 109 101 32 105 110 110 101 114 32 98 121 116 101 115 46 46 46] thing-4:false] in; map[thing-4:false thing-1:thing-1-value thing-2:800 thing-3:[115 111 109 101 32 105 110 110 101 114 32 98 121 116 101 115 46 46 46]] out
FAIL
FAIL    github.com/tinylib/msgp/msgp    0.147s

gobwas · 2016-11-19T11:56:39Z

@pwaller thanks =) Fixed.

gobwas · 2017-01-19T19:21:31Z

@philhofer may be today? 🤣

* Add (*Reader).CopyNext(w) (int64, error) It is useful to be able to efficiently copy objects without decoding them. My use case is filtering when I already know the indices of the objects I want to keep, and for rewriting a dictionary of objects as a column of objects. * Remove ReadNext * Add opportunistic optimization with m.R.Next * Remove unused ReadNextError * Remove commented code * small fixup - only call (*Reader).Next() when we're sure it won't realloc its buffer - promote io.ErrUnexpectedEOF to msgp.ErrShortBytes

potterdai · 2018-02-03T07:37:56Z

@philhofer Wondering if this PR will still be merged?

s.kamardin added 12 commits July 14, 2016 20:57

add ability to use numeric labels in map

493757e

readme fixes

fd9ac4f

bugfix

d57ab0a

fix tests

97823d4

bugfix with empty field tag

0108cd3

add generated files to clean task

9452270

declare vars out of the loop

6da8d3d

use declareOnce pattern

77232d3

optimize gen for single typed labels

79ca299

add tests for numeric labels

84311a7

code style

97f80c9

comment

f7c196e

philhofer reviewed Jul 15, 2016
View reviewed changes

s.kamardin added 6 commits July 22, 2016 14:02

use different syntax, refactoring

af829af

update readme

b2cdf6e

use different literals in readme example

91edff8

add different number literals test

62450e6

bug fix with single numeric field

56a4935

change panic message

26008c6

gobwas reviewed Jul 23, 2016
View reviewed changes

philhofer reviewed Aug 2, 2016
View reviewed changes

s.kamardin added 3 commits August 2, 2016 16:27

apply review comments

0c1cb94

common internal log package

7a547e1

use common log package

5fd38c2

fix import

efbe90c

s.kamardin added 3 commits September 14, 2016 17:56

ignore idea files

f1ccc2a

map keys are not just strings now

de20c15

apply gofmt to sources

dff3b85

s.kamardin added 2 commits September 14, 2016 18:07

eol

ff02952

fix possible shadowing bytes value in a map

af68dbd

s.kamardin added 3 commits October 18, 2016 14:12

read map of interfaces

61bf3ee

remove redundant errcheck

24d54ba

signature fix

9b93c50

s.kamardin added 2 commits November 19, 2016 14:29

map of interface to inteface

66e724c

fix tests, use map of interface instead

8eedca8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Struct fields numeric labels #160

Struct fields numeric labels #160

gobwas commented Jul 15, 2016 •

edited by philhofer

Loading

gobwas commented Jul 15, 2016 •

edited

Loading

philhofer commented Jul 15, 2016

philhofer commented Jul 15, 2016 •

edited

Loading

philhofer Jul 15, 2016

gobwas Jul 23, 2016

gobwas commented Jul 15, 2016

philhofer commented Jul 15, 2016

gobwas commented Jul 22, 2016 •

edited

Loading

gobwas Jul 23, 2016 •

edited

Loading

philhofer Aug 2, 2016

gobwas commented Jul 29, 2016

philhofer Aug 2, 2016

gobwas commented Aug 2, 2016 •

edited

Loading

gobwas commented Aug 8, 2016

gobwas commented Sep 14, 2016

gobwas commented Sep 14, 2016

gobwas commented Oct 17, 2016

pwaller commented Nov 11, 2016

gobwas commented Nov 19, 2016 •

edited

Loading

gobwas commented Jan 19, 2017

potterdai commented Feb 3, 2018

Struct fields numeric labels #160

Are you sure you want to change the base?

Struct fields numeric labels #160

Conversation

gobwas commented Jul 15, 2016 • edited by philhofer Loading

gobwas commented Jul 15, 2016 • edited Loading

philhofer commented Jul 15, 2016

philhofer commented Jul 15, 2016 • edited Loading

philhofer Jul 15, 2016

Choose a reason for hiding this comment

gobwas Jul 23, 2016

Choose a reason for hiding this comment

gobwas commented Jul 15, 2016

philhofer commented Jul 15, 2016

gobwas commented Jul 22, 2016 • edited Loading

gobwas Jul 23, 2016 • edited Loading

Choose a reason for hiding this comment

philhofer Aug 2, 2016

Choose a reason for hiding this comment

gobwas commented Jul 29, 2016

philhofer Aug 2, 2016

Choose a reason for hiding this comment

gobwas commented Aug 2, 2016 • edited Loading

gobwas commented Aug 8, 2016

gobwas commented Sep 14, 2016

gobwas commented Sep 14, 2016

gobwas commented Oct 17, 2016

pwaller commented Nov 11, 2016

gobwas commented Nov 19, 2016 • edited Loading

gobwas commented Jan 19, 2017

potterdai commented Feb 3, 2018

gobwas commented Jul 15, 2016 •

edited by philhofer

Loading

gobwas commented Jul 15, 2016 •

edited

Loading

philhofer commented Jul 15, 2016 •

edited

Loading

gobwas commented Jul 22, 2016 •

edited

Loading

gobwas Jul 23, 2016 •

edited

Loading

gobwas commented Aug 2, 2016 •

edited

Loading

gobwas commented Nov 19, 2016 •

edited

Loading