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: Add and use Station::HasFacilities(f). #8865

Closed
wants to merge 1 commit into from

Conversation

J0anJosep
Copy link
Contributor

Motivation / Problem

For some other PR I am working on, I have to check station facilities.
It would be much easier if I had Station::HasFacilities(f) at hand.

Description

The PR adds Station::HasFacilities(f) and it is used several times.

Limitations

Checklist for review

@LordAro
Copy link
Member

LordAro commented Mar 15, 2021

Codechanges for the sake of codechanges are generally discouraged, unless they provide some measurable benefit to maintenance, speed, or similar

This... (from our perspective at least) doesn't. It's not actually properly correct either - using StationFacility and a value that is not just a value out of the enum (i.e. a bitset, |, ~) is technically speaking, undefined behaviour. It should use byte as its type instead.

This should either be a commit in your eventual PR adding something new, or your welcome to reuse this PR when whatever you're doing is complete. But for now, I'm going to close it

@LordAro LordAro closed this Mar 15, 2021
@J0anJosep
Copy link
Contributor Author

J0anJosep commented Mar 15, 2021

Thanks.
I wanted to include this and some other codechanges to ease the review of other PRs and my work on them as well. I guess it is better to create a single big PR than smaller PRs.
About the incorrect use of StationFacility, if you can hint me how to correct it, I would be very obliged. If I have to discard it, better to discard it sooner than later. I thought that being StationFacility a byte and a bitset there would be no problem.

Edit: I guess in the script files the conversion is not right.

@J0anJosep J0anJosep deleted the HasFacilities branch November 22, 2021 21:24
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.

None yet

2 participants