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.

Sign up for GitHub

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

Jump to bottom

script_event_types.cpp inconsistant return values in EnginePreview #5802

Closed
DorpsGek opened this issue Nov 15, 2013 · 4 comments
Closed

script_event_types.cpp inconsistant return values in EnginePreview #5802

DorpsGek opened this issue Nov 15, 2013 · 4 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)

Comments

@DorpsGek
Copy link
Member

krinn opened the ticket and wrote:

ScriptEventEnginePreview::GetName
The worst, as it return null, and null value is a crash on many API functions that expect integer

ScriptEventEnginePreview::GetCargoType()
return CT_INVALID; // even it's better to return CT_INVALID, an invalid engine is not really a cargo

ScriptEventEnginePreview::GetCapacity()
return ScriptVehicle::VT_INVALID;

ScriptEventEnginePreview::GetMaxSpeed() & GetPrice() & GetRunningCost()
return -1;

I'm not sure if the return value would be use anyway as i don't see when the IsEngineValid could fail, i suppose openttd won't offer a preview to someone if the engine is not valid...
Kinda seems like a notreach() state, even wonder why the IsEngineValid check exist then.

But if the IsEngineValid check have a real reason, on failure in all the functions it should return a more consistant error : -1 for everyone or VT_INVALID, but not a mix like it is.

Reported version: trunk
Operating system: All

This issue was imported from FlySpray: https://bugs.openttd.org/task/5802
@DorpsGek
Copy link
Member Author

krinn wrote:

Forget to add -> inconstitant return value is only for the case when IsEngineValid check fails.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5802#comment12765

@DorpsGek
Copy link
Member Author

Rubidium wrote:

IsEngineValid is there for the case the player messes things up with (changing) NewGRFs. So, it's unlikely to happen but could and in that case it's better to let the AI crash than to crash OpenTTD.

GetName returns a string, which means a pointer. Returning -1 as pointer will cause crashes when you do not check for that. NULL is the universal way for saying there is no string. As such, NULL is the expected value for a 'no string'.

GetCargoType returns a cargo type. CT_INVALID is a value for a bad cargo type. -1 is not, especially if you do a check for Cargo.IsValid(ct) or so, or compare to CT_INVALID but not to -1.

GetCapacity returns an amount, -1 (which is returns, not VT_INVALID) is a good value for bad data. VT_INVALID is definitely not a good value as 255 is a valid capacity.

Likewise for the three other functions.

If you look at the rest of the APIs, you'll find plenty of cases where the 'bad data' value isn't a default -1. In all cases the actually chosen canary value should be better than just -1.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5802#comment12773

@DorpsGek
Copy link
Member Author

krinn wrote:

Ok thanks for taking time looking at it. I forget the add/remove newGRF case.
Sorry for the troubles.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5802#comment12774

@DorpsGek
Copy link
Member Author

Rubidium closed the ticket.

Reason for closing: Not a bug

This comment was imported from FlySpray: https://bugs.openttd.org/task/5802

@DorpsGek DorpsGek added flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) bug labels Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@DorpsGek