search

ropensci / parzer

Parse geographic coordinates
https://docs.ropensci.org/parzer
Other
63 stars 6 forks source link

support 0-360 #4

Open sckott opened 5 years ago

sckott commented 5 years ago

Looked into adjusting the C++ code to support 0-360, but not sure how to make it work exactly.

pretty sure this line https://github.com/ropenscilabs/parzer/blob/master/src/CLongLatString.cpp#L284 is what we need to change, but changing it to e.g., CheckRange(dDeg, 0.0, 360.0); then makes -360 valid, whereas we only want -180 through 360 valid. So seems like it's doing an abs on the data somewhere but I don't see that happening at least

I guess there's the approach of specifying whether your inputs are -180/180 or 0/360, but that seems not very convenient

sckott commented 5 years ago

any ideas @hollist @mpadge ?

mpadge commented 5 years ago

I'll have a look...

mpadge commented 5 years ago

Hmmm... I'm not really sure how the src code works here. Two things that appear potentially unsafe:

  1. nH = strCoord.find_first_not_of(" \t", 0); - unable to tell (without me delving deeper) if the first non-space will always denote sign? Could there be breaking use cases here?
  2. There is also, among others, this unsafe type conversion that may well generate unpredictable behaviour - I guess potential errors here are controlled elsewhere?
  3. There are a bunch of potential sign manipulations going on in these lines but I note that they would at least lead, for example, to "-20W" being parsed as "20W".

The unsafe type conversions in particular ought to be addressed in the interests of "best practice" - have you tried clang++ -Weverything on it? I imagine that would generate quite a barrage. Happy to help out if ya want - just give a shout!

sckott commented 5 years ago

thaks @mpadge ! I haven't tried clang++ -Weverything on it yet, will do if you can give me a hint of how to do that 😬

maybe your comments support #6 ? - that is, for re-writing the src side of the pkg perhaps. another motivation is the license - it's unclear to me what changing the code does with respect to the license. it's not a typically used license.

sckott commented 5 years ago

now working with the rewrite on branch src-rewrite