search

msoucy / dproto

D Protocol Buffer mixins to create structures at compile time
Boost Software License 1.0
37 stars 16 forks source link

Another attempt to fix issue #115. #130

Closed redstar closed 6 years ago

redstar commented 7 years ago

Root cause of the problem is that the used base data type for reading an integer is always ulong. Converting a (huge) value to a signed, possible shorter data type results in the onversion exception.

Solution is to use the correct type. This eliminates almost all calls to the std.conv.to function.

All credits for the fix go to @timotheecour Timothee Cour as he found the problem and created the first solution.

redstar commented 7 years ago

@timotheecour Maybe you can extend your PR #116 ? Because all credits for this belongs to you.

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling ca2f26631601de06fea98ae27c5a8be99eba253d on redstar:issue115 into on msoucy:master.

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling b86a929278ddd75d850543d20bc88d7a112d2aff on redstar:issue115 into on msoucy:master.

timotheecour commented 6 years ago

@redstar @msoucy what's the advantage over original PR https://github.com/msoucy/dproto/pull/116 ? The unittest still fails with "sint64" , "sint32" but you've omitted these cases (at least should document that these cases fail and how, as I was doing in orginal PR)

EDIT: I found how to fix the bug for "sint64" , "sint32" ; will update PR

redstar commented 6 years ago

@timotheecour This PR compiles with more DMD versions. Be sure to grab the __trait(compiles(...)) when you update your PR.

timotheecour commented 6 years ago

done, we can close this in favor of https://github.com/msoucy/dproto/pull/131 which also fixes the bug with "sint64" , "sint32"