-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modify pserver client C API, create better test. #2433
Conversation
Please refer to the change design doc for what in API have changed.
go/pserver/cclient/cclient.go
Outdated
for i := 0; i < int(total); i++ { | ||
if i >= len(ps) { | ||
break | ||
names := func() (string, string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that golang's strings#join can do the same thing?
https://golang.org/pkg/strings/#Join
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Makes the code much better! Done.
go/pserver/cclient/cclient.go
Outdated
|
||
if unsafe.Pointer(param) == nullPtr { | ||
param = (*C.paddle_parameter)(C.calloc(1, C.size_t(unsafe.Sizeof(*param)))) | ||
log.Println("must pre-allocate parameter.") | ||
return -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we define some error code? When I use this client, I have to search the code to make sure the meaning of the ret code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry! Added.
@@ -9,8 +9,10 @@ import ( | |||
// ElementType is the type of elements of a Parameter. | |||
type ElementType int | |||
|
|||
var ErrAlreadyInitialized = errors.New("pserver already initialized") | |||
var ErrUninitialized = errors.New("pserver not fully initialized") | |||
const ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the benefit of using const instead of errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, it's pretty tricky here.
Because if we are using
var ErrAlreadyInitialized = errors.New("pserver already initialized")
func Foo(a int b *int) error {
return ErrAlreadyInitialized
}
The RPC client can not compare the error:
err := client.Call("Service.Foo", 1, nil)
fmt.Println(err == ErrAlreadyInitialized)
// Output:
// false
This is happening because err
is of type error
, which is an interface.
Interface comparison rule:
A value x of non-interface type X and a value t of interface type T are comparable when values of type X are comparable and X implements T. They are equal if t's dynamic type is identical to X and t's dynamic value is equal to x.
Since the err
instance is deserialized by RPC, which is of different instance (dynamic value) than ErrAlreadyInitialized
, they are not equal.
Btw, comparing things like io.EOF
is typically fine:
_, err := os.Open("...")
if err == io.EOF {} // fine, os.Open return the same io.EOF instance.
if err == errors.New("EOF") {} // always false!
However, compare by string type is fine. Since comparison rule for string is just comparing the bytes. That why here is using string instead of error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get it, thanx
paddle_parameter* params[2] = {¶m_a, ¶m_b}; | ||
if (paddle_get_params(c, params, 2)) { | ||
fail(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we can check the value got from pserver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter value could change due to optimization happening on the pserver. So I did not check the value here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Please refer to the design doc diff in this PR for what in API have changed.