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

Fixed issue when TWKB does not paint geometries in high zoom levels (>18) #10

Merged
merged 2 commits into from
Nov 10, 2015

Conversation

dgaubert
Copy link

Resolved bad conversion from unsigned 64bit varInt when TWKB is enabled

Please @pramsey, could you review it?

cc @javisantana @rochoa

…signed 64bit varInt when TWKB is enabled
@pramsey
Copy link

pramsey commented Nov 10, 2015

(a) I think you reformatted the whitespace (spaces to tabs or just fewer spaces), which will get you yelled at when you submit patches to pgsql (as I learned) (it's anal retentive, but avoiding formatting changes makes patch review easier)
(b) I can't find any change in the actual logic of the routine. Yours is more compact but it reads to my addled brain like the same routine. Where was the mistake in the old one, what case did it fail on?

@dgaubert
Copy link
Author

(b) There is a little difference. In the old one, last iteration of "while loop" didn't perform:

nVal |= ((uint64_t)(nByte & 0x7f)) << nShift;

for high values (many iterations) this caused an ugly conversion (like an overflow error) in last iteration, due to:

return nVal | (uint64_t)(nByte << nShift);

(a) I found the following white-spacing format (tabs & spaces mixed):
previous
In last commit I've tried to keep the previous format:
final-format

@pramsey
Copy link

pramsey commented Nov 10, 2015

I'm still missing something, since the last step of the old version

return nVal | (uint64_t)(nByte << nShift);

seems equivalent to the intermediate steps,

nVal |= ((uint64_t)(nByte & 0x7f)) << nShift;

(since the final iteration has no hibit set, no "and" mask is required, and it just shifts, "or"s and returns) so I don't see where the overflow comes from.
That being said, you've tested it and it works, and your version looks fine to me, so +1.
(Wait, I see it! In the old version the return casts to uint64_t after the shift, not before, so Things Fall Apart there.)

@pramsey
Copy link

pramsey commented Nov 10, 2015

Incidentally the PostGIS code doesn't have this flaw, so I probably added it myself when transcribing it to mapnik, just goes to show that formatting anal retentiveness can lead to pain too.

pramsey added a commit that referenced this pull request Nov 10, 2015
Fixed issue when TWKB does not paint geometries in high zoom levels (>18)
@pramsey pramsey merged commit a22b3e5 into 2.3.x.cartodb Nov 10, 2015
@jgoizueta jgoizueta mentioned this pull request Jan 16, 2017
2 tasks
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