-
-
Notifications
You must be signed in to change notification settings - Fork 7k
Removed explicit check for true/false #7036
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, there's absolutely no reason to specify this explicitly. This should not affect the generated code in any way, so it seems safe to merge.
This change probably also needs to be applied to the SAM and SAMD cores, though.
Ok, should I make the changes on those files in this PR? (I can't seem to find the files, if you want me to make the changes, tell me where)
The other cores live in different repositories, so they'd need different PRs.
@facchinm, @cmaglie, what's the plan for these shared files? I remember some plans to extract them into a shared space, but I'm not sure how far along those plans are? Regardless, what's the best course to take here? Preparing separate PRs, or just copying over the entire Stream.cpp file at some later point?
@matthijskooijman You said this was safe to merge a few months ago. Any chance of this actually happening? This is a minor change, so it shouldn't affect much. (Sorry to pester you)
Well, since you've pinged them, that's good.
Hi @arnavb , sure, we are in the middle of a transition to https://github.com/arduino/ArduinoCore-avr so I'd prefer not to merge the PR here but to merge it there. Furthermore, as you can see from the "Print and Stream class" tag, it is not an hardware related change so it's going to be merged as part of Chainsaw project. Sorry for the confusion about all these repos but we are trying to provide on a more modern way to contribute to the project.
@facchinm Makes sense! Thanks for the info!
The title says it all.