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

Codechange: encapsulate network error handling (and fix #9142) #9146

Merged
merged 3 commits into from May 1, 2021

Conversation

rubidium42
Copy link
Contributor

@rubidium42 rubidium42 commented Apr 30, 2021

Closes #9142.

Motivation / Problem

The discussion about #9142 and how to solve that nicely (and permanently) yielded another rabbit hole.
This does solve #9142, but it should also prevent it from happening again as doing it another way would break the compile for at least Windows.

Description

Create a NetworkError class that has functions to query what kind of error it is. This way the actual definition of what a particular error entails needs to be coded only once. #9142 is a nice example of why the current method is not sustainable; if someone else adds a new place where recv/send is used, and would just use err == EWOULDBLOCK then #9142 happens again. By abstracting the whole error type determination to a single location, it needs to be set only once and can then be reused.

To not have huge amounts of complicated #ifdefs in os_abstraction.h, the actual implementation has been placed in os_abstraction.cpp and some of the functions defined in os_abstraction.h have been moved as well.
By this refactor the #ifdefs for the functionality are also moved into the functions themselves, so for one function you can easily see the implementation for the different environments, whereas you had to really look well through os_abstraction.h before to find out what would happen exactly in different environments.

Limitations

None, as far as I know.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@rubidium42 rubidium42 changed the title Network error handling Codechange: encapsulate network error handling (and fix #9142) Apr 30, 2021
@rubidium42 rubidium42 added the backport requested This PR should be backport to current release (RC / stable) label Apr 30, 2021
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine; I only wonder if we shouldn't just put this in the header-file, so we can let compilers optimize it away immediately. Otherwise I guess it needs some LTO to do it properly. No clue if it matters either way.

Also, I would like a second opinion on the C++ side of this, but yeah .. I think this is the better solution to go with, as we don't force every OS into a Linux-like experience :D

src/network/core/os_abstraction.h Outdated Show resolved Hide resolved
src/network/core/os_abstraction.cpp Outdated Show resolved Hide resolved
src/network/core/os_abstraction.cpp Outdated Show resolved Hide resolved
src/network/core/os_abstraction.cpp Outdated Show resolved Hide resolved
src/network/core/os_abstraction.cpp Outdated Show resolved Hide resolved
@LordAro
Copy link
Member

LordAro commented May 1, 2021

I realise my previous comments somewhat contradict this, but given (almost?) every use of NetworkError::AsString/GetLastAsString is in DEBUG() that need a const char*... maybe that would be a better return type? :> Would save the very noisy c_str() call every time...

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is fine by me

@rubidium42 rubidium42 merged commit e097c83 into OpenTTD:master May 1, 2021
@rubidium42 rubidium42 deleted the network_error_handling branch May 1, 2021 17:36
@LordAro LordAro added backported This PR is backported to a current release (RC / stable) and removed backport requested This PR should be backport to current release (RC / stable) labels May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR is backported to a current release (RC / stable)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants