search

BurntSushi / xgbutil

A utility library to make use of the X Go Binding easier. (Implements EWMH and ICCCM specs, key binding support, etc.)
Do What The F*ck You Want To Public License
194 stars 46 forks source link

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

Open snyh opened 10 years ago

snyh commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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