Skip to content

Comments

Address -Wunused-result compiler warning#2232

Closed
BadAssassin wants to merge 3 commits intoOSGeo:mainfrom
BadAssassin:unused-result
Closed

Address -Wunused-result compiler warning#2232
BadAssassin wants to merge 3 commits intoOSGeo:mainfrom
BadAssassin:unused-result

Conversation

@BadAssassin
Copy link
Contributor

QOL update to address usused results from C library functions.

@wenzeslaus
Copy link
Member

Thanks @BadAssassin! This looks like #2166. Can you perhaps help there by reviewing it and comparing to your code?

@BadAssassin
Copy link
Contributor Author

BadAssassin commented Feb 24, 2022 via email

@wenzeslaus
Copy link
Member

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.

@wenzeslaus
Copy link
Member

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.

@wenzeslaus wenzeslaus added this to the 8.4.0 milestone Apr 28, 2022
@neteler
Copy link
Member

neteler commented May 20, 2022

Re-opening this: above it was suggested to close but still #2166 is open, too. Please align both, thanks.

@neteler neteler reopened this May 20, 2022
@BadAssassin
Copy link
Contributor Author

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.

@wenzeslaus
Copy link
Member

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).

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.

3 participants