grass
grass copied to clipboard
OSGeo
Address -Wunused-result compiler warning
QOL update to address usused results from C library functions.
Thanks @BadAssassin! This looks like #2166. Can you perhaps help there by reviewing it and comparing to your code?
This is, indeed, a duplicated effort.
Should I revert my PR? I am unsure of how to handle this.
On 2/23/2022 5:41 PM, Vaclav Petras wrote:
Thanks @BadAssassin https://github.com/BadAssassin! This looks like #2166 https://github.com/OSGeo/grass/pull/2166. Can you perhaps help there by reviewing it and comparing to your code?
Should I revert my PR? I am unsure of how to handle this.
No need to do anything in this PR. You can just close it and that's it. I could close it myself, but I thought you may prefer to check the other PR first whether it fixes all you fixed here.
Unrelated to the code, but I saw the two other commit on your branch which are unrelated, so I check your fork, particularly what you have on the main branch. You committed the two legend commits to your main branch. You can see it here:
https://github.com/BadAssassin/grass/commits/main?after=ce304a633859c940227b9fbe8fe4f6d0c97ffed2+34&branch=main
So now, when you create a new branch from main, it always contains these two commits. Something I should have checked when we talked by email.
This also explains why you have all the Merge branch 'OSGeo:main' into main commits on your main branch (which I saw somewhere in some PR too). You can't now update cleanly (with git rebase or git pull) because there are these two extra commits, so a merge commit needs to be created.
You need to hard reset your local main branch to the OSGeo/grass main branch. For me, it would be git reset --hard upstream/main, but check with git remote -v what you should put there in place of upstream. Taking your precious changes and putting them outside of Git is advisable.
Re-opening this: above it was suggested to close but still #2166 is open, too. Please align both, thanks.
Thank you, everyone for your comments. It was my first attempt at using the new github setup (I remember the old SVN servers!). The issues have been corrected. My apologies for any inconveniences I may have caused.
I have reviewed #2166. That one actually has better error handling output than mine. Otherwise, they are nearly identical. However both patches do suffer from a couple math errors (as noted in the commit).
If you would like to commit #2166, close this, then I will fix the math conversion errors in a future commit.
Thanks @BadAssassin. Given that you reviewed #2166 and consider #2166 a better one, I will close this one. It has conflicts with main due to change in indentation on main which can't be easily resolved, so using #2166 is better from that perspective too (although I'm guessing there is some good for it like a different or additional fix).