Confusion with bool conversion on absl::optional<bool>

[Yuwei Huang]

The bool operator on abs::optional returns true as long as it holds a value, which can be quite confusing when people instantiate it with bool, say:

absl::optional<bool> some_bool = false;

if (some_bool) {

  LOG(INFO) << "Hey!";  // This will be logged.

}

This could be especially problematic if the optional is defined in some header file and the developer uses it without realizing it's an optional instead of a plain old bool.

I wonder if it makes sense to add some presubmit warning for using the bool operator on absl::optional<bool>, or maybe just ban it altogether and force people to use has_value() or value() instead? 

[Aaron Tagliaboschi]

I'd really just question the introduction of an optional boolean on review. Is there an example of this in Chromium? I'm really struggling to come up with an example where it would be useful.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/f19ee7ea-793c-478f-a452-abd570d3be57n%40chromium.org.

[Sylvain Defresne]

There is usage of absl::optional<bool> in Chromium such as base::Value::FindBoolKey (and similar functions). The goal is to allow returning a base::nullopt value if the key is not found, or a bool with the correct value otherwise. I think this is a valid use case for absl::optional<bool> (though error prone).

The subject has already been discussed on this list in past thread, the summary was:

1. if possible use an enum with three possible values "undefined", "yes", "no" if possible,

2. prefer using `has_value()` or `value_or(default)` instead of dependending on the conversion, and limit scope of the optional,

3. google style and internal google code do not ban optional<bool> though they point they are error prone.

I think having a clang plugin warning for the use of `absl::optional<bool>::operator bool() const` would be a good idea since we have alternatives (.has_value() / .or_value(default)). I am not sure a presubmit would be possible as there are valid use cases for absl::optional<bool>.

-- Sylvain

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJwB4XbkTQ9SAQGbah0Ao3J%2BKVdnROcgFO9-C%2B2mP7k7-4Nd0w%40mail.gmail.com.

[Yuwei Huang]

Yeah, I wouldn’t ban the usage of optional<bool>, but having something that warns about the usage of operator bool() would be good.

[Ryan Hamilton]

Agreed. Optional<bool> seems useful if uncommon, but operator bool() in this case seems dangerous to me. 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAC3a%3DDO-parsjaypjY_2gDze%3DrujSucOhR0qbR7EQ7PYfyB9bw%40mail.gmail.com.

[Lei Zhang]

This topic came up a few months ago on chromium-dev:
https://groups.google.com/a/chromium.org/g/chromium-dev/c/F43x0MO_tEU/m/AmBbCdxxAQAJ

[Danil Chapovalov]

This topic is also covered in https://abseil.io/tips/141

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACSHbcTQe3Poy%3Dka1AHcBcG-jmR0wPHN4Mi4HnBtymumBW058g%40mail.gmail.com.

[Ryo Hashimoto]

+1 for having a warning against operator bool().

Even when I know it's an optional<bool>, it's really difficult to avoid this confusion when reading the code.

Also it might be worth having the same warning for optional<T*> and other types that frequently get converted to bool (though I guess it happens much less frequently compared to optional<bool>).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACQca%3DcTShvBLGKc%2BTaupeAmW%3Duh_veLCZai_MUmUDuDByn%2BxA%40mail.gmail.com.

[Gabriel Charette]

Agreed that optional<T> and in particular optional<T*> has the same problem really. I think it comes down to variable naming? i.e. Don't call an optional<bool> "is_enabled" but rather "enabled_state" or "optional_status" and ideally use StrongAlias to qualify the bool as an explicit type.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CABtzobJM5AyYOEZJ0nE1OHbxy38i9Cq6gEw77us%3DJmeOfT6sdg%40mail.gmail.com.