Skip to content

Conversation

@pratikpc
Copy link

@pratikpc pratikpc commented Feb 1, 2019

If you were to notice that in every Serial based Class, we have a function with the type
operator bool() { return true; }

By writing explicit, we can ensure that there is no form of implicit conversion or otherwise.
For further details view https://stackoverflow.com/questions/39995573/when-can-i-use-explicit-operator-bool-without-a-cast

This would ensure that bool b = Serial1; would not result in b getting a value

For example, comparing the following codes:-

struct T {
    operator bool() const { return true; }
};
int main()
{
    T t;
    bool B = t;
}

Does not result in an error due to an absence of explicit.
However, adding explicit we get an error that the conversion cannot be performed in the following sample.

struct T {
    explicit operator bool() const { return true; }
};
int main()
{
    T t;
    bool B = t; // error cannot convert t from bool
}

An Operator bool with explicit would render it compatible with only these statements

For example

while(Serial);
if(Serial)
for(;Serial;)

CONSTEXPR

Further, I have marked these statements as constexpr given they indeed are constant compile time expressions and their values will not change dynamically at runtime

@bxparks
Copy link
Contributor

bxparks commented Feb 4, 2019

Drive-by comment:

I'm concerned that this potentially breaks user code, if someone did something like this:

bool status = Serial1 && (some other long boolean expression);
if (status) { ... }

I'm always in favor of making C++ less dangerous. But, is breaking of backwards compatibility worth it here? Is allowing bool b = Serial1 that dangerous?

Also, it seems to me that we would need to change the operator bool() in the following files for consistency:

hardware/arduino/avr/cores/arduino/Client.h
hardware/arduino/avr/cores/arduino/USBAPI.h
hardware/arduino/avr/libraries/SoftwareSerial/src/SoftwareSerial.h

Additionally, how do we handle the 3rd party Cores (e.g. Teensy, ESP8266, ESP32, etc) which have implemented the Stream or Serial interface, and copied the operator bool() method? For consistency, don't we want to update those as well? Or are you proposing a fork in the API from those other cores?

@PaulStoffregen
Copy link
Contributor

Additionally, how do we handle the 3rd party Cores (e.g. Teensy, ESP8266, ESP32, etc) which have implemented the Stream or Serial interface, and copied the operator bool() method?

Hopefully opening issues or pull requests on those other repositories isn't considered to be too much trouble? The time to do so is when Arduino merges this change. But don't expect any of us to actually do much at that moment. The time to "bump" the issue with another comment is when Arduino makes a non-beta release with this feature.

I can't speak for the ESP cores, but I can tell regarding Teensy... I do pay attention to Arduino development. I generally see changes like this long before they're merged. :-)

@pratikpc
Copy link
Author

pratikpc commented Feb 5, 2019

@bxparks actually that example will not break.
Implicit will only break direct conversion.
For example, check out the following example on Godbolt

struct X
{
   constexpr explicit operator bool() const
   {
      return true;
   }
};

int main()
{
   X Serial;
   bool B = Serial && 34 == 34;
}

This example would run and work because this is not an implicit conversion but rather a form of checking a condition.

So don't worry... None of yours or the existing communities code would end up breaking!!!!
It would only break if you were doing something like
bool B = Serial;
This without a doubt looks fishy

I also plan on adding this Pull Request on Multiple Arduino based Repos.
I have already added a few to the official Arduino based repos as well as to Teensy

@pratikpc pratikpc closed this Feb 5, 2019
@pratikpc pratikpc reopened this Feb 5, 2019
@pratikpc
Copy link
Author

pratikpc commented Feb 5, 2019

@bxparks updated with support for all 4

@CLAassistant
Copy link

CLAassistant commented Apr 9, 2021

CLA assistant check
All committers have signed the CLA.

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