fix https://github.com/msoucy/dproto/issues/115

msoucy / dproto

D Protocol Buffer mixins to create structures at compile time

Boost Software License 1.0

37 stars 16 forks source link

fix https://github.com/msoucy/dproto/issues/115 #116

Closed timotheecour closed 6 years ago

timotheecour commented 7 years ago

this should fix https://github.com/msoucy/dproto/issues/115.

side question for @msoucy:

please see comment in diff: should i use readVarint!(Unsigned!T2)() instead of src.readVarint() in static if(T == "sint32" || T == "sint64") block? currently, it'll force to use ulong and then call fromZigZag().to!T2(); (ie convert to a int or long) with the code in comment, it'll read into a uint or ulong, then call zigzag to convert to int or long.

coveralls commented 7 years ago

Changes Unknown when pulling 69776bdda70e92fa64dc0af2be7a3e80ccb55dca on timotheecour:fix_115_overflow into on msoucy:master.

timotheecour commented 7 years ago

@msoucy I just added a unittest; it shows additional bugs for types: "sint64" , "sint32"; however, these are pre-existing bugs so shouldn't block this PR, but I'm curious what you think would fix it?

coveralls commented 7 years ago

Changes Unknown when pulling c7209fd381d84239b85f526cd445e21866b9b2cb on timotheecour:fix_115_overflow into on msoucy:master.

coveralls commented 7 years ago

Changes Unknown when pulling 909454f26a1d412fb8986c7dd3a0009277e63d8f on timotheecour:fix_115_overflow into on msoucy:master.

timotheecour commented 7 years ago

@msoucy seems like the travis failures are ok: works on recent version of dmd,ldc; should we maybe remove old versions where it fails?

msoucy commented 7 years ago

I'm never sure which versions of dmd to with Travis... It's too bad that there isn't a way to specify "the versions that are available to the package managers for commonly used distros".

I'll make that update to .travis.yml, but I'm not sure that it'll help this. Versions of LDC that are accessible by supported releases (without adding other sources, of course) are 0.16.1 (Fedora 24, DMD frontend 2.067.1), 0.17.3 (Fedora 25, DMD frontend 2.068.2), and 1.1.1 (Fedora 26, DMD frontend 2.071.2). I can't find the versions of the DMD frontend supported by the Debian packages, though, only the GCC backend. It looks like the DMD frontend supported by its most recent version is 2.068.2, though.

Though I haven't been following this as strictly as I should have, I would prefer that dproto is usable on all compilers that are installable by default on Fedora and Debian.

timotheecour commented 7 years ago

any comments on this PR? can we ignore the travis failures since https://travis-ci.org/msoucy/dproto shows git master is also failing for some targets?

redstar commented 7 years ago

PR #123 fixes all Travis failures. After merging this PR should be green, too.

coveralls commented 7 years ago

Changes Unknown when pulling c6a564124054f2df389894c40dedf6cd05287665 on timotheecour:fix_115_overflow into on msoucy:master.

coveralls commented 7 years ago

Changes Unknown when pulling 5bf0a182f7f1d60dcca96ce02db7fe3c8fce9840 on timotheecour:fix_115_overflow into on msoucy:master.

redstar commented 7 years ago

@timotheecour Please have a look at #130. I think you can fix your PR with code from mine.

timotheecour commented 6 years ago

done, closing in favor of https://github.com/msoucy/dproto/pull/131