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

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

Closed
wants to merge 5 commits into from

Conversation

timotheecour
Copy link
Contributor

this should fix #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
Copy link

Coverage Status

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

@timotheecour
Copy link
Contributor Author

timotheecour commented Jun 10, 2017

@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
Copy link

coveralls commented Jun 10, 2017

Coverage Status

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

@coveralls
Copy link

coveralls commented Jun 11, 2017

Coverage Status

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

Repository owner deleted a comment from coveralls Jun 11, 2017
Repository owner deleted a comment from coveralls Jun 11, 2017
@timotheecour
Copy link
Contributor Author

@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
Copy link
Owner

msoucy commented Jun 12, 2017

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.

msoucy added a commit that referenced this pull request Jun 13, 2017
@timotheecour
Copy link
Contributor Author

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
Copy link
Contributor

redstar commented Aug 18, 2017

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

@coveralls
Copy link

coveralls commented Aug 19, 2017

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@redstar
Copy link
Contributor

redstar commented Aug 29, 2017

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

@timotheecour
Copy link
Contributor Author

done, closing in favor of #131

Repository owner deleted a comment from coveralls Jan 26, 2018
Repository owner deleted a comment from coveralls Jan 26, 2018
Repository owner deleted a comment from coveralls Jan 26, 2018
Repository owner deleted a comment from coveralls Jan 26, 2018
Repository owner deleted a comment from coveralls Jan 26, 2018
Repository owner deleted a comment from coveralls Jan 26, 2018
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.

BUG:Conversion positive overflow for negative integers (works in C++, fails in D)
4 participants