Feat/decode simple java class and base data type #18
Conversation
…va-class-and-base-data-type
|
pls fix the ci |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ChangedenCZD, jasondeng1997 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
pls fix the ci |
|
run |
|
|
||
| - name: Unit Test | ||
| run: go test -race -covermode=atomic -coverprofile=coverage.out ./... |
There was a problem hiding this comment.
Please keep the -race flag
| @@ -1,23 +0,0 @@ | |||
| name: Action Test | |||
There was a problem hiding this comment.
Why deleting action.yml and main.yml?
| @@ -9,7 +9,7 @@ jobs: | |||
| - uses: actions/checkout@v3 | |||
|
|
|||
| - name: Check License Header | |||
| uses: apache/skywalking-eyes/header@main | |||
There was a problem hiding this comment.
why changing to the v0.4.0?
typos.toml
Outdated
There was a problem hiding this comment.
Why renaming this file? Better keep the files generated from the template (cloudwego/.github) as is.
decoder.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| ReadSize = 1024 |
There was a problem hiding this comment.
- Please add comments for all exported symbols.
- It seems this constant could be a non-exported symbol.
| case tag == BC_FALSE: | ||
| return false | ||
|
|
||
| // direct integer |
There was a problem hiding this comment.
看起来这块的实现和 readInt、readDouble 里都有大量的重复，建议这样重构一下：
readField()方法先读取 tag，然后解析该字段的值read<Type>()调用 readField() 然后再分发处理
例如：
func (d *Decoder) readField() (tp fieldType, value interface{}) {
tag = d.readTag()
switch (tag) {
case BC_TRUE:
return TYPE_BOOL, d.parseBool()
case BC_DATE:
return TYPE_DATE, d.parseLong()
// all the rest tag cases
}
}
func (d *Decoder) readBool() bool {
tp, value := d.readField()
switch(tp) {
case BC_BOOL:
return value.(bool)
// deal with other types
...
}
}
There was a problem hiding this comment.
我看到后面有 readObject()，和我前面说的 readField() 类似，但考虑到不同的 hessian2 类型可能会返回相同的基础数据类型（例如 UTCDate 也返回 int64），readField除了返回value，还应该返回一个 hessian 类型
| return d.read() != 0 | ||
|
|
||
| // INT_BYTE != 0 | ||
| case (tag >= BYTE_INT && tag <= BYTE_INT_MAX) || |
There was a problem hiding this comment.
对于 readBool 函数，为什么要把其他类型也当成 bool 来处理呢？有具体的场景需求吗
| if tag == BC_DATE { | ||
| return d.parseLong() | ||
| } else if tag == BC_DATE_MINUTE { | ||
| return int64(d.parseInt()) |
There was a problem hiding this comment.
- 这里是不是应该 * 60 * 1000 ? 否则调用者可能无法区分单位是 ms 还是 minute 。
- 返回类型为什么不是 time.Time? （和注释不一致）
|
I will rewrite this part of the code |
decoder.go
Outdated
| case tag >= DIRECT_INT && tag <= DIRECT_INT_MAX: | ||
| return int(tag) - BC_INT_ZERO | ||
|
|
||
| /* byte int */ |
There was a problem hiding this comment.
Is there any difference between the comment with // and one with /**/?
| case tag >= SHORT_INT && tag <= SHORT_INT_LIMIT_MAX: | ||
| return ((int(tag) - BC_INT_SHORT_ZERO) << 16) + 256*int(d.read()) + int(d.read()) | ||
|
|
||
| case tag == BC_INT || tag == BC_LONG_INT: |
There was a problem hiding this comment.
Why sometimes there's no comment? Maybe you can unify the style for readability.
|
close this pr |
|
We've proposed to implement the encoder/decoder in a more efficient way in #28. |
What type of PR is this?
Check the PR title.
(Optional) Translate the PR title into Chinese.
(Optional) More detailed description for this PR(en: English/zh: Chinese).
en:
zh(optional):
(Optional) Which issue(s) this PR fixes:
(optional) The PR that updates user documentation: