ropensci / parzer

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

pz_split_llstr_string compiler warning #26

Closed sckott closed 4 years ago

sckott commented 4 years ago

Getting compiler warning on Windows for this method, see https://github.com/ropensci/parzer/runs/870437962?check_suite_focus=true#step:11:76

pz_split_llstr.cpp:19:69: warning: suggest parentheses around comparison in operand of '|' [-Wparentheses]
   } else if (nbCommas == 0 && nbSpaces == 1 && nbSC == 0 && (nbDots == 0 | nbDots == 2)) {
                                                              ~~~~~~~^~~~

I tried to just put another set of parens around the existing parens i.e., ((nbDots == 0 | nbDots == 2)) , but that didn't work, not sure what the problem is

ping @AlbanSagouis

p.s., switched to using github actions, you can find checks at https://github.com/ropensci/parzer/actions?query=workflow%3AR-check

AlbanSagouis commented 4 years ago

Hi @sckott Yes, I had this warning when checking too but I couldn't see how to solve it and the script was compiling and working as intended so I committed anyway.

Now that I had an other look, I thought the issue was not with the parens but with the |, there should be 2 and there was only one, my bad. I just pushed a commit solving it.

sckott commented 4 years ago

Thanks!

AlbanSagouis commented 4 years ago

Hi @sckott The modification was so small that I wonder. What would have been best practice between writing a comment with the modified line of code included and clear info on where it should go OR modifying the code in my repo and then requesting a pull request?

sckott commented 4 years ago

writing a comment

does that mean opening an issue?

If the question is: "open an issue or PR" - I'd say open an issue first unless the change is sufficiently small OR if it's larger but doesn't touch code (e.g, if you add documentation only or make a change to a non-code file).

In this case there was an issue already open (this one).

For any code changes, I always locally run tests and R check to make sure no problems were introduced, and then CI will do that too after pushing up. Additionally, for any code changes that there isn't currently a test for, ideally add a new test

AlbanSagouis commented 4 years ago

By 'writing a comment', I meant sending you a message on the page of this already opened issue and just paste the modified line of code.

Thanks for the advice!

sckott commented 4 years ago

Ah, yes, you can definitely do that