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

Replaced macros with constants. #28

Merged
merged 2 commits into from
Feb 15, 2022

Conversation

lewie-donckers
Copy link
Contributor

@Timple
Copy link
Member

Timple commented Feb 15, 2022

I'm surprised this passes the build.

Note to self: check why roslint is not active, there should be two spaces before the comment.

@rokusottervanger
Copy link
Contributor

It's an improvement. But more important may be to check why these are constants and not parameters in the first place.

@lewie-donckers
Copy link
Contributor Author

It's an improvement. But more important may be to check why these are constants and not parameters in the first place.

I suggest you have someone with more domain knowledge than me create an issue about this.

@lewie-donckers
Copy link
Contributor Author

I'm surprised this passes the build.

Note to self: check why roslint is not active, there should be two spaces before the comment.

Is this something you want fixed?

(I'm not using the clang-format yet.)

@rokusottervanger
Copy link
Contributor

rokusottervanger commented Feb 15, 2022

It's an improvement. But more important may be to check why these are constants and not parameters in the first place.

I suggest you have someone with more domain knowledge than me create an issue about this.

You're right. This discussion is already going on in #2 . I'll create an issue about magic numbers in general: #35

@lewie-donckers lewie-donckers force-pushed the cleanup/switched-to-enum-classes branch from 1c6b7f8 to 1b54e83 Compare February 15, 2022 13:52
@lewie-donckers lewie-donckers force-pushed the cleanup/replaced-macros-with-constants branch from e07ccef to a992823 Compare February 15, 2022 13:52
@Timple
Copy link
Member

Timple commented Feb 15, 2022

Is this something you want fixed?

I want the build to fail on this :).

But yes, please fix as well.

@lewie-donckers lewie-donckers changed the base branch from cleanup/switched-to-enum-classes to main February 15, 2022 15:05
Copy link
Contributor

@Rayman Rayman left a comment

Choose a reason for hiding this comment

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

Nice, constants are definitely much better than macros

@lewie-donckers
Copy link
Contributor Author

Is this something you want fixed?

I want the build to fail on this :).

But yes, please fix as well.

Fixed by formatting those lines according to the .clang-format file provided by @Timple.

@lewie-donckers lewie-donckers merged commit 14b5e5e into main Feb 15, 2022
@lewie-donckers lewie-donckers deleted the cleanup/replaced-macros-with-constants branch February 16, 2022 08:43
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.

4 participants