Refactor explicit operator bool usage for some cases #8601

brunoabinader · 2017-03-31T14:47:14Z

No description provided.

jfirebaugh

So, to add some additional nuance to #8562 (comment), not all our uses of operator bool() are implementing a validity check (though some of them definitely are). I would distinguish:

Use of member data sentinel values to create an ad hoc implementation of an "absent" state, e.g. in PositionedIcon, Shaping, Glyph, GlyphMetrics. These should typically be replaced with uses of optional<T> and elimination of the "invalid" state from the inner type.
Types which are acting similarly to a pointer or optional type, e.g. ExtensionFunction, CFHandle. These could arguably use optional too, but sometimes it's not convenient. Emulating a pointer by providing operator bool() (and usually operator* as well) is reasonable.
Result. This is in its own category because it's a fundamental type, on the level of optional. Providing operator bool() is necessary for error-handling ergonomics.
Types which have some other special state, but where that state is considered a valid member of the type's domain. These should typically use a named method specific to the state, e.g. TransitionOptions::present() or PropertyValue::defined(). (Note that the most natural name might name the complement of the special state -- all members of the type's domain other than the special state.)

jfirebaugh · 2017-03-31T18:37:15Z

include/mbgl/style/property_value.hpp

@@ -34,7 +34,7 @@ class PropertyValue {
    const                T & asConstant()       const { return value.template get<               T >(); }
    const CameraFunction<T>& asCameraFunction() const { return value.template get<CameraFunction<T>>(); }

-    explicit operator bool() const { return !isUndefined(); };
+    bool valid() const { return !isUndefined(); };


defined or present would be better; an undefined property value isn't invalid.

This is actually not used, it seems isUndefined() suits the purpose already.

jfirebaugh · 2017-03-31T18:37:23Z

include/mbgl/style/transition_options.hpp

@@ -18,7 +18,7 @@ class TransitionOptions {
        };
    }

-    explicit operator bool() const {
+    bool valid() const {


jfirebaugh · 2017-03-31T18:38:52Z

include/mbgl/util/geo.hpp

@@ -48,7 +48,7 @@ class LatLng {
        else if (longitude < 0 && end.longitude > 0) longitude += util::DEGREES_MAX;
    }

-    explicit operator bool() const {
+    bool valid() const {


#3802 tracks removing the invalid state from these classes entirely.

Sure, I'm going to defer these to #3802.

jfirebaugh · 2017-03-31T18:39:35Z

include/mbgl/util/size.hpp

@@ -17,7 +17,7 @@ class Size {
        return width * height;
    }

-    constexpr explicit operator bool() const {
+    constexpr bool valid() const {


Reverse the sense and call it empty or zero instead?

Deferring this to #8562.

jfirebaugh · 2017-03-31T18:44:33Z

platform/android/src/conversion/conversion.hpp

@@ -17,7 +17,7 @@ class Result : private variant<T, Error> {
 public:
    using variant<T, Error>::variant;

-    explicit operator bool() const {
+    bool valid() const {


I think this is a reasonable use of operator bool and doesn't need to change. Of course, we should probably follow d7227e1 and eliminate this copy of Result entirely.

jfirebaugh · 2017-03-31T18:46:56Z

platform/default/sqlite3.cpp

@@ -158,8 +158,8 @@ Statement &Statement::operator=(Statement &&other) {
 Statement::~Statement() {
 }

-Statement::operator bool() const {
-    return impl.operator bool();
+bool Statement::valid() const {


jfirebaugh · 2017-03-31T18:48:06Z

src/mbgl/gl/extension.hpp

@@ -18,7 +18,7 @@ class ExtensionFunction<R(Args...)> {
    ExtensionFunction(const ProcAddress ptr_) : ptr(ptr_) {
    }

-    explicit operator bool() const {
+    bool valid() const {


I think this is a valid use of operator bool and doesn't need to change. ExtensionFunction acts like a function pointer.

jfirebaugh · 2017-03-31T18:48:57Z

src/mbgl/layout/symbol_instance.cpp

@@ -23,34 +23,37 @@ SymbolInstance::SymbolInstance(Anchor& anchor,
                               const std::size_t featureIndex_) :
    point(anchor.point),
    index(index_),
-    hasText(shapedTextOrientations.first || shapedTextOrientations.second),
-    hasIcon(shapedIcon),
+    hasText(shapedTextOrientations.first.valid() || shapedTextOrientations.second.valid()),


A better fix here is likely to use an optional.

jfirebaugh · 2017-03-31T18:50:55Z

src/mbgl/text/glyph.hpp

@@ -16,7 +16,7 @@ namespace mbgl {
 GlyphRange getGlyphRange(char16_t glyph);

 struct GlyphMetrics {
-    explicit operator bool() const {
+    bool valid() const {


If "all 0" is being used as a proxy for "no metrics available", a better change is to use optional<GlyphMetrics>.

jfirebaugh · 2017-03-31T18:51:29Z

src/mbgl/text/glyph.hpp

@@ -42,8 +42,8 @@ struct Glyph {
    explicit Glyph(Rect<uint16_t> rect_, GlyphMetrics metrics_)
        : rect(std::move(rect_)), metrics(std::move(metrics_)) {}

-    explicit operator bool() const {
-        return metrics || rect.hasArea();
+    bool valid() const {


Same here -- optional<Glyph> instead.

brunoabinader · 2017-04-03T17:08:57Z

@jfirebaugh let's wait for #3802 and #8562 to land, so I can rebase and address the review comments in one step.

brunoabinader · 2017-04-13T15:22:45Z

@jfirebaugh I've applied the requested changes based on your comments - thank you for the detailed explaining on these!

The only exception is Shaping, which starts failing some render tests once I replace the explicit operator bool usage with optional. I'm going to skip it for now and get back to it once I figure out exactly why in some cases the symbols are not being inserted into the buckets when the text shaping is not valid.

jfirebaugh · 2017-04-13T19:53:44Z

src/mbgl/text/glyph_pbf.hpp

@@ -21,7 +21,7 @@ class SDFGlyph {
    AlphaImage bitmap;

    // Glyph metrics
-    GlyphMetrics metrics;
+    optional<GlyphMetrics> metrics;


Once Glyph is optional, GlyphMetrics::operator bool() is unused, so we don't need to make this optional.

I have an alternate fix for glyph-related operator bool() prepared as part of #8747, so I'll omit the last commit here and merge.

Thank you @jfirebaugh 👍

brunoabinader added the Core The cross-platform C++ core, aka mbgl label Mar 31, 2017

brunoabinader requested review from kkaefer and jfirebaugh March 31, 2017 14:47

brunoabinader changed the title ~~[all] Prefer 'valid()' over 'explicit operator bool()'~~ [all] Prefer valid() over explicit operator bool() Mar 31, 2017

brunoabinader changed the title ~~[all] Prefer valid() over explicit operator bool()~~ [all] Prefer valid() over explicit operator bool() Mar 31, 2017

brunoabinader mentioned this pull request Mar 31, 2017

Tighten size check #8562

Merged

brunoabinader force-pushed the rip-explicit-operator-bool branch 2 times, most recently from 54ba98f to b411b8b Compare March 31, 2017 15:08

jfirebaugh suggested changes Mar 31, 2017

View reviewed changes

brunoabinader mentioned this pull request Apr 3, 2017

[core] Tighten LatLng and other geo.hpp class invariants #5286

Merged

brunoabinader force-pushed the rip-explicit-operator-bool branch 2 times, most recently from a9c52fd to e677563 Compare April 12, 2017 16:15

brunoabinader changed the title ~~[all] Prefer valid() over explicit operator bool()~~ Refactor explicit operator bool usage for some cases Apr 12, 2017

brunoabinader force-pushed the rip-explicit-operator-bool branch from e677563 to e84594d Compare April 12, 2017 16:22

brunoabinader added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Apr 12, 2017

brunoabinader force-pushed the rip-explicit-operator-bool branch from e84594d to f3f7ec0 Compare April 13, 2017 14:50

brunoabinader removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Apr 13, 2017

jfirebaugh suggested changes Apr 13, 2017

View reviewed changes

brunoabinader added 4 commits April 14, 2017 11:59

[core] Remove unused bool() functions

c1bb7b7

[core] TransitionOptions s/operator bool/isDefined/

ee3e7d0

[core] Tighten geo.hpp ctors

55015ce

[core] Safeguard PositionedIcon usage via optional

04fb715

jfirebaugh approved these changes Apr 14, 2017

View reviewed changes

jfirebaugh force-pushed the rip-explicit-operator-bool branch from f3f7ec0 to 04fb715 Compare April 14, 2017 19:05

jfirebaugh merged commit 98e2e59 into master Apr 14, 2017

jfirebaugh deleted the rip-explicit-operator-bool branch April 14, 2017 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor explicit operator bool usage for some cases #8601

Refactor explicit operator bool usage for some cases #8601

brunoabinader commented Mar 31, 2017

jfirebaugh left a comment •

edited

Loading

jfirebaugh Mar 31, 2017

brunoabinader Apr 11, 2017

jfirebaugh Mar 31, 2017

jfirebaugh Mar 31, 2017

brunoabinader Apr 3, 2017

jfirebaugh Mar 31, 2017

brunoabinader Apr 3, 2017

jfirebaugh Mar 31, 2017

jfirebaugh Mar 31, 2017

jfirebaugh Mar 31, 2017

jfirebaugh Mar 31, 2017

jfirebaugh Mar 31, 2017

jfirebaugh Mar 31, 2017

brunoabinader commented Apr 3, 2017

brunoabinader commented Apr 13, 2017

jfirebaugh Apr 13, 2017

jfirebaugh Apr 14, 2017

brunoabinader Apr 18, 2017

Refactor explicit operator bool usage for some cases #8601

Refactor explicit operator bool usage for some cases #8601

Conversation

brunoabinader commented Mar 31, 2017

jfirebaugh left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunoabinader commented Apr 3, 2017

brunoabinader commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfirebaugh left a comment •

edited

Loading