Skip to content
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

use number2XX to convert safely number value from interface{} #20

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

snyh
Copy link
Contributor

@snyh snyh commented Mar 28, 2014

use type assertion directly is dangerous, it will be panic when use ewmh.ShowingDesktopReq now.

the xpop.ChangePorperty need data with uint type, NewClientMessage assume it data with int type, and there are all use interface{} to pass data. so compiled ok then run panic.

It can also be use reflect package to reduce the code, but I think simply use an type switch is enough.

@BurntSushi
Copy link
Owner

So, I think this isn't a terrible idea, but I'm not sure your solution is less dangerous than what was already there. In particular, if the format is 8 and an int is given, this patch will silently convert it to a byte which is likely to produce incorrect behavior without an error. In that case, the panic is definitely preferred I think.

I wouldn't be opposed to making the function more flexible though. For example, right now if the format is 16, then only int16 is accepted. But we could also accept uint16. For 32, we could accept uint32, int32, int and uint. For 8, we could accept byte and uint8. If there's a mismatch, then we can panic with a more helpful error.

Reflection definitely isn't appropriate in this case.

How does that sound? I'd be happy to do the changes for you. Let me know.

@snyh
Copy link
Contributor Author

snyh commented Apr 3, 2014

Of course it sounds more appropriately. Thank for your explaining.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants