fix: treat warnings as errors in CI (#253)

Many users might compile the SDK using a "warnings as errors" flag. 

To support this use-case, update CI to compile the sdk with CMAKE_COMPILE_WARNING_AS_ERROR=TRUE. This commit also resolves existing warnings, with the exception of OpenSSL deprecation warnings which have been supressed using CMake generator expressions.

Author: cwaldren-ld

.clang-format

@@ -2,4 +2,5 @@
 BasedOnStyle: Chromium
 IndentWidth: 4
 QualifierAlignment: Right
+NamespaceIndentation: None
 ...

cmake/expected.cmake

@@ -7,9 +7,10 @@ if (${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.24")
     cmake_policy(SET CMP0135 NEW)
 endif ()
 
+set(EXPECTED_BUILD_TESTS OFF)
 FetchContent_Declare(tl-expected
         GIT_REPOSITORY https://github.com/TartanLlama/expected.git
         GIT_TAG 292eff8bd8ee230a7df1d6a1c00c4ea0eb2f0362
-        )
+)
 
 FetchContent_MakeAvailable(tl-expected)

libs/client-sdk/src/bindings/c/builder.cpp

@@ -93,7 +93,7 @@ LDClientConfigBuilder_Build(LDClientConfigBuilder b,
     LD_ASSERT_NOT_NULL(b);
     LD_ASSERT_NOT_NULL(out_config);
 
-    return launchdarkly::ConsumeBuilder<ConfigBuilder>(b, out_config);
+    return launchdarkly::detail::ConsumeBuilder<ConfigBuilder>(b, out_config);
 }
 
 LD_EXPORT(void)

libs/client-sdk/src/data_sources/streaming_data_source.cpp

@@ -1,3 +1,11 @@
+#include "streaming_data_source.hpp"
+
+#include <launchdarkly/context_builder.hpp>
+#include <launchdarkly/detail/unreachable.hpp>
+#include <launchdarkly/encoding/base_64.hpp>
+#include <launchdarkly/network/http_requester.hpp>
+#include <launchdarkly/serialization/json_context.hpp>
+
 #include <boost/asio/any_io_executor.hpp>
 #include <boost/asio/io_context.hpp>
 #include <boost/asio/post.hpp>
@@ -6,13 +14,6 @@
 
 #include <utility>
 
-#include "streaming_data_source.hpp"
-
-#include <launchdarkly/context_builder.hpp>
-#include <launchdarkly/encoding/base_64.hpp>
-#include <launchdarkly/network/http_requester.hpp>
-#include <launchdarkly/serialization/json_context.hpp>
-
 namespace launchdarkly::client_side::data_sources {
 
 static char const* const kCouldNotParseEndpoint =
@@ -30,6 +31,7 @@ static char const* DataSourceErrorToString(launchdarkly::sse::Error error) {
         case sse::Error::ReadTimeout:
             return "read timeout reached";
     }
+    launchdarkly::detail::unreachable();
 }
 
 StreamingDataSource::StreamingDataSource(

libs/common/include/launchdarkly/attributes.hpp

@@ -118,8 +118,8 @@ class Attributes final {
         : key_(std::move(key)),
           name_(std::move(name)),
           anonymous_(anonymous),
-          custom_attributes_(std::move(attributes)),
-          private_attributes_(std::move(private_attributes)) {}
+          private_attributes_(std::move(private_attributes)),
+          custom_attributes_(std::move(attributes)) {}
 
     friend std::ostream& operator<<(std::ostream& out,
                                     Attributes const& attrs) {
@@ -142,9 +142,13 @@ class Attributes final {
     }
 
     Attributes(Attributes const& context) = default;
+
     Attributes(Attributes&& context) = default;
+
     ~Attributes() = default;
+
     Attributes& operator=(Attributes const&) = default;
+
     Attributes& operator=(Attributes&&) = default;
 
    private:

libs/common/include/launchdarkly/detail/c_binding_helpers.hpp

@@ -6,7 +6,7 @@
 
 #include <tl/expected.hpp>
 
-namespace launchdarkly {
+namespace launchdarkly::detail {
 template <typename T, typename = void>
 struct has_result_type : std::false_type {};
 
@@ -105,5 +105,5 @@ bool OptReturnReinterpretCast(std::optional<OptType>& opt,
 
 #define LD_ASSERT_NOT_NULL(param) LD_ASSERT(param != nullptr)
 
-}  // namespace launchdarkly
+}  // namespace launchdarkly::detail
 // NOLINTEND cppcoreguidelines-pro-type-reinterpret-cast

libs/common/include/launchdarkly/detail/unreachable.hpp

@@ -0,0 +1,14 @@
+namespace launchdarkly::detail {
+
+// This may be replaced with a standard routine when C++23 is available.
+[[noreturn]] inline void unreachable() {
+// Uses compiler specific extensions if possible.
+// Even if no extension is used, undefined behavior is still raised by
+// an empty function body and the noreturn attribute.
+#if defined(__GNUC__)  // GCC, Clang, ICC
+    __builtin_unreachable();
+#elif defined(_MSC_VER)  // MSVC
+    __assume(false);
+#endif
+}
+}  // namespace launchdarkly::detail

libs/common/include/launchdarkly/value.hpp

@@ -119,7 +119,7 @@ class Value final {
             using iterator_category = std::forward_iterator_tag;
             using difference_type = std::ptrdiff_t;
 
-            using value_type = std::pair<const std::string, Value>;
+            using value_type = std::pair<std::string const, Value>;
             using pointer = value_type const*;
             using reference = value_type const&;
 
@@ -374,7 +374,7 @@ class Value final {
     static Value const& Null();
 
     friend std::ostream& operator<<(std::ostream& out, Value const& value) {
-        switch (value.type_) {
+        switch (value.Type()) {
             case Type::kNull:
                 out << "null()";
                 break;
@@ -409,16 +409,17 @@ class Value final {
     operator int() const { return AsInt(); }
 
    private:
-    std::variant<bool, double, std::string, Array, Object> storage_;
-    enum Type type_;
+    struct null_type {};
+
+    std::variant<null_type, bool, double, std::string, Array, Object> storage_;
 
     // Empty constants used when accessing the wrong type.
     // These are not inline static const because of this bug:
     // https://developercommunity.visualstudio.com/t/inline-static-destructors-are-called-multiple-time/1157794
-    static const std::string empty_string_;
-    static const Array empty_vector_;
-    static const Object empty_map_;
-    static const Value null_value_;
+    static std::string const empty_string_;
+    static Array const empty_vector_;
+    static Object const empty_map_;
+    static Value const null_value_;
 };
 
 bool operator==(Value const& lhs, Value const& rhs);

libs/common/src/attribute_reference.cpp

@@ -229,10 +229,11 @@ AttributeReference::AttributeReference(char const* ref_str)
 std::string AttributeReference::PathToStringReference(
     std::vector<std::string_view> path) {
     // Approximate size to reduce resizes.
-    auto size = std::accumulate(path.begin(), path.end(), 0,
-                                [](auto sum, auto const& component) {
-                                    return sum + component.size() + 1;
-                                });
+    std::size_t size =
+        std::accumulate(path.begin(), path.end(), std::size_t{0},
+                        [](std::size_t sum, auto const& component) {
+                            return sum + component.size() + 1;
+                        });
 
     std::string redaction_name;
     redaction_name.reserve(size);

libs/common/src/bindings/c/data/evaluation_detail.cpp

@@ -11,6 +11,7 @@
 #define FROM_REASON(ptr) (reinterpret_cast<LDEvalReason>(ptr));
 
 using namespace launchdarkly;
+using namespace launchdarkly::detail;
 
 LD_EXPORT(void)
 LDEvalDetail_Free(LDEvalDetail detail) {

libs/common/src/bindings/c/value.cpp

@@ -4,6 +4,7 @@
 #include <launchdarkly/bindings/c/value.h>
 #include <launchdarkly/bindings/c/iter.hpp>
 #include <launchdarkly/detail/c_binding_helpers.hpp>
+#include <launchdarkly/detail/unreachable.hpp>
 #include <launchdarkly/value.hpp>
 
 using launchdarkly::Value;
@@ -59,7 +60,7 @@ LD_EXPORT(enum LDValueType) LDValue_Type(LDValue val) {
         case Value::Type::kArray:
             return LDValueType_Array;
     }
-    LD_ASSERT(!"Unsupported value type.");
+    launchdarkly::detail::unreachable();
 }
 
 LD_EXPORT(bool) LDValue_GetBool(LDValue val) {

libs/common/src/value.cpp

@@ -1,63 +1,80 @@
-#pragma clang diagnostic push
-
 #include <launchdarkly/value.hpp>
 
 #include <cstring>
 #include <iostream>
 
 namespace launchdarkly {
 
-const std::string Value::empty_string_;
-const Value::Array Value::empty_vector_;
-const Value::Object Value::empty_map_;
-const Value Value::null_value_;
+std::string const Value::empty_string_;
+Value::Array const Value::empty_vector_;
+Value::Object const Value::empty_map_;
+Value const Value::null_value_;
+
+Value::Value() : storage_{null_type{}} {}
 
-Value::Value() : type_(Value::Type::kNull), storage_{0.0} {}
+Value::Value(bool boolean) : storage_{boolean} {}
 
-Value::Value(bool boolean) : type_(Value::Type::kBool), storage_{boolean} {}
+Value::Value(double num) : storage_{num} {}
 
-Value::Value(double num) : type_(Value::Type::kNumber), storage_{num} {}
+Value::Value(int num) : storage_{(double)num} {}
 
-Value::Value(int num) : type_(Value::Type::kNumber), storage_{(double)num} {}
+Value::Value(std::string str) : storage_{std::move(str)} {}
 
-Value::Value(std::string str)
-    : type_(Value::Type::kString), storage_{std::move(str)} {}
+Value::Value(char const* str) : storage_{std::string(str)} {}
 
-Value::Value(char const* str)
-    : type_(Value::Type::kString), storage_{std::string(str)} {}
+Value::Value(std::vector<Value> arr) : storage_{std::move(arr)} {}
 
-Value::Value(std::vector<Value> arr)
-    : type_(Value::Type::kArray), storage_{std::move(arr)} {}
+Value::Value(std::map<std::string, Value> obj) : storage_{std::move(obj)} {}
 
-Value::Value(std::map<std::string, Value> obj)
-    : type_(Value::Type::kObject), storage_{std::move(obj)} {}
+template <class>
+inline constexpr bool always_false_v = false;
 
 enum Value::Type Value::Type() const {
-    return type_;
+    return std::visit(
+        [](auto const& arg) {
+            using T = std::decay_t<decltype(arg)>;
+            if constexpr (std::is_same_v<T, null_type>) {
+                return Type::kNull;
+            } else if constexpr (std::is_same_v<T, bool>) {
+                return Type::kBool;
+            } else if constexpr (std::is_same_v<T, double>) {
+                return Type::kNumber;
+            } else if constexpr (std::is_same_v<T, std::string>) {
+                return Type::kString;
+            } else if constexpr (std::is_same_v<T, Value::Array>) {
+                return Type::kArray;
+            } else if constexpr (std::is_same_v<T, Value::Object>) {
+                return Type::kObject;
+            } else {
+                static_assert(always_false_v<T>,
+                              "all value types must be visited");
+            }
+        },
+        storage_);
 }
 
 bool Value::IsNull() const {
-    return type_ == Type::kNull;
+    return std::holds_alternative<null_type>(storage_);
 }
 
 bool Value::IsBool() const {
-    return type_ == Type::kBool;
+    return std::holds_alternative<bool>(storage_);
 }
 
 bool Value::IsNumber() const {
-    return type_ == Type::kNumber;
+    return std::holds_alternative<double>(storage_);
 }
 
 bool Value::IsString() const {
-    return type_ == Type::kString;
+    return std::holds_alternative<std::string>(storage_);
 }
 
 bool Value::IsArray() const {
-    return type_ == Type::kArray;
+    return std::holds_alternative<Value::Array>(storage_);
 }
 
 bool Value::IsObject() const {
-    return type_ == Type::kObject;
+    return std::holds_alternative<Value::Object>(storage_);
 }
 
 Value const& Value::Null() {
@@ -67,62 +84,60 @@ Value const& Value::Null() {
 }
 
 bool Value::AsBool() const {
-    if (type_ == Type::kBool) {
+    if (IsBool()) {
         return std::get<bool>(storage_);
     }
     return false;
 }
 
 int Value::AsInt() const {
-    if (type_ == Type::kNumber) {
+    if (IsNumber()) {
         return static_cast<int>(std::get<double>(storage_));
     }
     return 0;
 }
 
 double Value::AsDouble() const {
-    if (type_ == Type::kNumber) {
+    if (IsNumber()) {
         return std::get<double>(storage_);
     }
     return 0.0;
 }
 
 std::string const& Value::AsString() const {
-    if (type_ == Type::kString) {
+    if (IsString()) {
         return std::get<std::string>(storage_);
     }
     return empty_string_;
 }
 
 Value::Array const& Value::AsArray() const {
-    if (type_ == Type::kArray) {
+    if (IsArray()) {
         return std::get<Array>(storage_);
     }
     return empty_vector_;
 }
 
 Value::Object const& Value::AsObject() const {
-    if (type_ == Type::kObject) {
+    if (IsObject()) {
         return std::get<Object>(storage_);
     }
     return empty_map_;
 }
 
 Value::Value(std::optional<std::string> opt_str) : storage_{0.0} {
     if (opt_str.has_value()) {
-        type_ = Type::kString;
         storage_ = opt_str.value();
     } else {
-        type_ = Type::kNull;
+        storage_ = null_type{};
     }
 }
 Value::Value(std::initializer_list<Value> values)
-    : type_(Type::kArray), storage_(std::vector<Value>(values)) {}
+    : storage_(std::vector<Value>(values)) {}
+
+Value::Value(Value::Array arr) : storage_(std::move(arr)) {}
 
-Value::Value(Value::Array arr)
-    : storage_(std::move(arr)), type_(Type::kArray) {}
-Value::Value(Value::Object obj)
-    : storage_(std::move(obj)), type_(Type::kObject) {}
+Value::Value(Value::Object obj) : storage_(std::move(obj)) {}
 
 Value::Array::Iterator::Iterator(std::vector<Value>::const_iterator iterator)
     : iterator_(iterator) {}