Code safety/modernization (#234)

* Xcode: Enabled -Wimplicit-fallthrough * CompilerSupport / PlatformCompat overhaul - Removed obsolete/unused MUST_USE_RESULT - Changed LITECORE_UNUSED to be the standard C++17 [[maybe_unused]] and commented that we should stop using the macro - Added LIFETIMEBOUND Removed LITECORE_UNUSED, FLEECE_UNUSED Replaced with standard [[maybe_unused]] * Use LIFETIMEBOUND in API Some slice code cleanup, removing C++11/C++14 shims - Added `data()` and `size_bytes()` methods to slice types, like those in std::span. (Can't add `size()` because it conflicts with a data member.) This is an incremental step toward moving to `span`, or at least making the `buf` and `size` fields private. - Likewise, added `data()`, `size_bytes()` to FLSlice and FLSliceResult, as well as `empty()` and `operator bool`. - Removed obsolete `sliceHash`. - Removed conditional support for `std::string_view`, unnecessary since we require C++17. - Removed `constexpr14` which was only useful when we supported C++11. - Added default initializers to slice member variables instead of initializing in the default constructor. - Use `= default` for default constructors. * C++ improvements to Fleece.hh - Make Value, Array, Dict trivially copyable: this allows instances to be passed in a register instead of on the stack in some ABIs, such as ARM64, which is more efficient. (The fix is to remove the custom operator= methods, which are unnecessary.) - Made operator==, operator!= methods const. - Made move constructors/assignment `noexcept`, which helps collections generate more optimal code. - Don't pass `alloc_slice` or `string` by value. * Removed unnecessary #include <algorithm> from RefCounted.hh --------- Co-authored-by: Jianmin Zhao <[email protected]>
couchbase · Oct 7, 2024 · 170d717 · 170d717
1 parent aea5c0c
commit 170d717
Show file tree

Hide file tree

Showing 26 changed files with 224 additions and 214 deletions.
diff --git a/API/fleece/CompilerSupport.h b/API/fleece/CompilerSupport.h
@@ -14,13 +14,22 @@
 #ifndef _FLEECE_COMPILER_SUPPORT_H
 #define _FLEECE_COMPILER_SUPPORT_H
 
+#ifdef __APPLE__
+#include <sys/cdefs.h>  // include this first to avoid conflict with our definition of __printflike
+#endif
+
+
 // The __has_xxx() macros are supported by [at least] Clang and GCC.
 // Define them to return 0 on other compilers.
 // https://clang.llvm.org/docs/AttributeReference.html
 // https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html
 
 #ifndef __has_attribute
-    #define __has_attribute(x) 0
+#define __has_attribute(x) 0
+#endif
+
+#ifndef __has_cpp_attribute
+#define __has_cpp_attribute(x) 0
 #endif
 
 #ifndef __has_builtin
@@ -43,24 +52,22 @@
 #  define RETURNS_NONNULL
 #endif
 
-// deprecated; use NODISCARD instead
-#if __has_attribute(returns_nonnull)
-#  define MUST_USE_RESULT               __attribute__((warn_unused_result))
-#else
-#  define MUST_USE_RESULT
-#endif
 
-// NODISCARD expands to the C++17/C23 `[[nodiscard]]` attribute, or else MUST_USE_RESULT.
-// (We can't just redefine MUST_USE_RESULT as `[[nodiscard]]` unfortunately, because the former is
-// already in use in positions where `[[nodiscard]]` isn't valid, like at the end of a declaration.)
+// NODISCARD expands to the C++17/C23 `[[nodiscard]]` attribute.
+// Use it before a function declaration when it's a mistake to ignore the function's result.
 #if (__cplusplus >= 201700L) || (__STDC_VERSION__ >= 202000)
 #  define NODISCARD                     [[nodiscard]]
+#elif __has_attribute(warn_unused_result)
+#  define NODISCARD                     __attribute__((warn_unused_result))
 #else
-#  define NODISCARD                     MUST_USE_RESULT
+#  define NODISCARD
 #endif
 
+
 // These have no effect on behavior, but they hint to the optimizer which branch of an 'if'
 // statement to make faster.
+// Note: In C++20 the standard attributes `[[likely]]` and `[[unlikely]]` can be used instead,
+// but they're not syntactically identical.
 #if __has_builtin(__builtin_expect)
 #define _usuallyTrue(VAL)               __builtin_expect(VAL, true)
 #define _usuallyFalse(VAL)              __builtin_expect(VAL, false)
@@ -75,7 +82,7 @@
 // disallow NULL values, unless annotated with FL_NULLABLE (which must come after the `*`.)
 // (FL_NONNULL is occasionally necessary when there are multiple levels of pointers.)
 // NOTE: Only supported in Clang, so far.
-#if defined(__clang__)
+#if __has_feature(nullability)
 #  define FL_ASSUME_NONNULL_BEGIN _Pragma("clang assume_nonnull begin")
 #  define FL_ASSUME_NONNULL_END _Pragma("clang assume_nonnull end")
 #  define FL_NULLABLE _Nullable
@@ -146,18 +153,6 @@
 #endif
 
 
-// `constexpr14` is for uses of `constexpr` that are valid in C++14 but not earlier.
-// In constexpr functions this includes `if`, `for`, `while` statements; or multiple `return`s.
-// The macro expands to `constexpr` in C++14 or later, otherwise to nothing.
-#ifdef __cplusplus
-    #if __cplusplus >= 201400L || _MSVC_LANG >= 201400L
-        #define constexpr14 constexpr
-    #else
-        #define constexpr14
-    #endif
-#endif // __cplusplus
-
-
 // STEPOVER is for trivial little glue functions that are annoying to step into in the debugger
 // on the way to the function you _do_ want to step into. Examples are RefCounted's operator->,
 // or slice constructors. Suppressing debug info for those functions means the debugger
@@ -279,6 +274,29 @@
     #define FLAPI
 #endif
 
+// `LIFETIMEBOUND` helps Clang detect value lifetime errors, where one value's lifetime is tied to another and the
+// dependent value is used after the value it depends on exits scope. For examples of usage, see slice.hh.
+// "The `lifetimebound` attribute on a function parameter or implicit object parameter indicates that objects that are
+// referred to by that parameter may also be referred to by the return value of the annotated function (or, for a
+// parameter of a constructor, by the value of the constructed object)."
+// -- https://clang.llvm.org/docs/AttributeReference.html#lifetimebound
+#if __has_cpp_attribute(clang::lifetimebound)
+#   define LIFETIMEBOUND [[clang::lifetimebound]]
+#else
+#   define LIFETIMEBOUND
+#endif
+
+
+// Type-checking for printf-style vararg functions:
+#ifndef __printflike
+#   if __has_attribute(__format__)
+#        define __printflike(fmtarg, firstvararg) __attribute__((__format__(__printf__, fmtarg, firstvararg)))
+#   else
+#       define __printflike(A, B)
+#   endif
+#endif
+
+
 #else // _FLEECE_COMPILER_SUPPORT_H
 #warn "Compiler is not honoring #pragma once"
 #endif
diff --git a/API/fleece/FLDoc.h b/API/fleece/FLDoc.h
@@ -48,13 +48,13 @@ extern "C" {
     FLEECE_PUBLIC FLDoc FLDoc_Retain(FLDoc FL_NULLABLE) FLAPI;
 
     /** Returns the encoded Fleece data backing the document. */
-    FLEECE_PUBLIC FLSlice FLDoc_GetData(FLDoc FL_NULLABLE) FLAPI FLPURE;
+    FLEECE_PUBLIC FLSlice FLDoc_GetData(FLDoc FL_NULLABLE doc LIFETIMEBOUND) FLAPI FLPURE;
 
     /** Returns the FLSliceResult data owned by the document, if any, else a null slice. */
     FLEECE_PUBLIC FLSliceResult FLDoc_GetAllocedData(FLDoc FL_NULLABLE) FLAPI FLPURE;
 
     /** Returns the root value in the FLDoc, usually an FLDict. */
-    FLEECE_PUBLIC FLValue FLDoc_GetRoot(FLDoc FL_NULLABLE) FLAPI FLPURE;
+    FLEECE_PUBLIC FLValue FLDoc_GetRoot(FLDoc FL_NULLABLE doc LIFETIMEBOUND) FLAPI FLPURE;
 
     /** Returns the FLSharedKeys used by this FLDoc, as specified when it was created. */
     FLEECE_PUBLIC FLSharedKeys FLDoc_GetSharedKeys(FLDoc FL_NULLABLE) FLAPI FLPURE;

diff --git a/API/fleece/FLEncoder.h b/API/fleece/FLEncoder.h
@@ -213,7 +213,6 @@ extern "C" {
 
     /** Ends encoding; if there has been no error, it returns the encoded data, else null.
         This does not free the FLEncoder; call FLEncoder_Free (or FLEncoder_Reset) next. */
-    NODISCARD
     FLEECE_PUBLIC FLSliceResult FLEncoder_Finish(FLEncoder, FLError* FL_NULLABLE outError) FLAPI;
 
     /** @} */
@@ -226,7 +225,7 @@ extern "C" {
     FLEECE_PUBLIC FLError FLEncoder_GetError(FLEncoder) FLAPI;
 
     /** Returns the error message of an encoder, or NULL if there's no error. */
-    FLEECE_PUBLIC const char* FL_NULLABLE FLEncoder_GetErrorMessage(FLEncoder) FLAPI;
+    FLEECE_PUBLIC const char* FL_NULLABLE FLEncoder_GetErrorMessage(FLEncoder e LIFETIMEBOUND) FLAPI;
 
     /** @} */
     /** @} */

diff --git a/API/fleece/FLSlice.h b/API/fleece/FLSlice.h
@@ -47,8 +47,11 @@ typedef struct FLSlice {
     size_t size;
 
 #ifdef __cplusplus
-    explicit operator bool() const noexcept FLPURE  {return buf != nullptr;}
-    explicit operator std::string() const           {return std::string((char*)buf, size);}
+    constexpr const void* FL_NULLABLE data() const noexcept FLPURE  {return buf;}
+    constexpr size_t size_bytes() const noexcept FLPURE             {return size;} // compatibility with std::span
+    constexpr bool empty() const noexcept FLPURE                    {return size == 0;}
+    constexpr explicit operator bool() const noexcept FLPURE        {return buf != nullptr;}
+    explicit operator std::string() const                           {return std::string((char*)buf, size);}
 #endif
 } FLSlice;
 
@@ -65,8 +68,11 @@ struct NODISCARD FLSliceResult {
     size_t size;
 
 #ifdef __cplusplus
-    explicit operator bool() const noexcept FLPURE  {return buf != nullptr;}
-    explicit operator FLSlice () const              {return {buf, size};}
+    constexpr const void* FL_NULLABLE data() const noexcept FLPURE  {return buf;}
+    constexpr size_t size_bytes() const noexcept FLPURE             {return size;} // compatibility with std::span
+    constexpr bool empty() const noexcept FLPURE                    {return size == 0;}
+    constexpr explicit operator bool() const noexcept FLPURE        {return buf != nullptr;}
+    constexpr explicit operator FLSlice () const                    {return {buf, size};}
     inline explicit operator std::string() const;
 #endif
 };
@@ -123,7 +129,7 @@ inline void FLMemCpy(void* FL_NULLABLE dst, const void* FL_NULLABLE src, size_t
     It's OK to pass NULL; this returns an empty slice.
     \note If the string is a literal, it's more efficient to use \ref FLSTR instead.
     \note Performance is O(n) with the length of the string, since it has to call `strlen`. */
-inline FLSlice FLStr(const char* FL_NULLABLE str) FLAPI {
+inline FLSlice FLStr(const char* FL_NULLABLE str LIFETIMEBOUND) FLAPI {
     FLSlice foo = { str, str ? strlen(str) : 0 };
     return foo;
 }
@@ -185,7 +191,7 @@ inline void FLSliceResult_Release(FLSliceResult s) FLAPI {
 }
 
 /** Type-casts a FLSliceResult to FLSlice, since C doesn't know it's a subclass. */
-inline FLSlice FLSliceResult_AsSlice(FLSliceResult sr) {
+inline FLSlice FLSliceResult_AsSlice(FLSliceResult sr LIFETIMEBOUND) {
     FLSlice ret;
     memcpy(&ret, &sr, sizeof(ret));
     return ret;

diff --git a/API/fleece/Fleece.hh b/API/fleece/Fleece.hh
@@ -79,7 +79,6 @@ namespace fleece {
 
         bool isEqual(Value v) const                     {return FLValue_IsEqual(_val, v);}
 
-        Value& operator= (Value v) &                    {_val = v._val; return *this;}
         Value& operator= (std::nullptr_t) &             {_val = nullptr; return *this;}
 
         inline Value operator[] (const KeyPath &kp) const;
@@ -129,9 +128,8 @@ namespace fleece {
         inline Value operator[] (int index) const       {return get(index);}
         inline Value operator[] (const KeyPath &kp) const {return Value::operator[](kp);}
 
-        Array& operator= (Array a) &                     {_val = a._val; return *this;}
-        Array& operator= (std::nullptr_t) &              {_val = nullptr; return *this;}
-        Value& operator= (Value v)                       =delete;
+        Array& operator= (std::nullptr_t) &             {_val = nullptr; return *this;}
+        Value& operator= (Value v)                      =delete;
 
         [[nodiscard]] inline MutableArray asMutable() const;
 
@@ -149,7 +147,7 @@ namespace fleece {
             inline Value operator * () const            {return value();}
             inline explicit operator bool() const       {return (bool)value();}
             inline iterator& operator++ ()              {next(); return *this;}
-            inline bool operator!= (const iterator&)    {return value() != nullptr;}
+            inline bool operator!= (const iterator&) const    {return value() != nullptr;}
             inline Value operator[] (unsigned n) const  {return FLArrayIterator_GetValueAt(this,n);}
         private:
             iterator() =default;
@@ -187,7 +185,6 @@ namespace fleece {
         inline Value operator[] (const char *key) const {return get(key);}
         inline Value operator[] (const KeyPath &kp) const {return Value::operator[](kp);}
 
-        Dict& operator= (Dict d) &                      {_val = d._val; return *this;}
         Dict& operator= (std::nullptr_t) &              {_val = nullptr; return *this;}
         Value& operator= (Value v)                      =delete;
 
@@ -261,8 +258,8 @@ namespace fleece {
         KeyPath(slice_NONNULL spec, FLError* FL_NULLABLE err) :_path(FLKeyPath_New(spec, err)) { }
         ~KeyPath()                                      {FLKeyPath_Free(_path);}
 
-        KeyPath(KeyPath &&kp)                           :_path(kp._path) {kp._path = nullptr;}
-        KeyPath& operator=(KeyPath &&kp) &              {FLKeyPath_Free(_path); _path = kp._path;
+        KeyPath(KeyPath &&kp) noexcept                  :_path(kp._path) {kp._path = nullptr;}
+        KeyPath& operator=(KeyPath &&kp) & noexcept     {FLKeyPath_Free(_path); _path = kp._path;
                                                          kp._path = nullptr; return *this;}
 
         KeyPath(const KeyPath &kp)                      :KeyPath(std::string(kp), nullptr) { }
@@ -334,7 +331,7 @@ namespace fleece {
         external pointers to. */
     class Doc {
     public:
-        Doc(alloc_slice fleeceData,
+        Doc(const alloc_slice& fleeceData,
             FLTrust trust =kFLUntrusted,
             FLSharedKeys FL_NULLABLE sk =nullptr,
             slice externDest =nullslice) noexcept
@@ -356,23 +353,23 @@ namespace fleece {
         Doc& operator=(Doc &&other) noexcept;
         ~Doc()                                      {FLDoc_Release(_doc);}
 
-        slice data() const                          {return FLDoc_GetData(_doc);}
+        slice data() const LIFETIMEBOUND            {return FLDoc_GetData(_doc);}
         alloc_slice allocedData() const             {return FLDoc_GetAllocedData(_doc);}
         FLSharedKeys sharedKeys() const             {return FLDoc_GetSharedKeys(_doc);}
 
-        Value root() const                          {return FLDoc_GetRoot(_doc);}
+        Value root() const LIFETIMEBOUND            {return FLDoc_GetRoot(_doc);}
         explicit operator bool () const             {return root() != nullptr;}
         Array asArray() const                       {return root().asArray();}
         Dict asDict() const                         {return root().asDict();}
 
-        operator Value () const                     {return root();}
-        operator Dict () const                      {return asDict();}
-        operator FLDict FL_NULLABLE () const        {return asDict();}
+        operator Value () const LIFETIMEBOUND                     {return root();}
+        operator Dict () const LIFETIMEBOUND                      {return asDict();}
+        operator FLDict FL_NULLABLE () const LIFETIMEBOUND        {return asDict();}
 
-        Value operator[] (int index) const          {return asArray().get(index);}
-        Value operator[] (slice key) const          {return asDict().get(key);}
-        Value operator[] (const char *key) const    {return asDict().get(key);}
-        Value operator[] (const KeyPath &kp) const  {return root().operator[](kp);}
+        Value operator[] (int index) const LIFETIMEBOUND          {return asArray().get(index);}
+        Value operator[] (slice key) const LIFETIMEBOUND          {return asDict().get(key);}
+        Value operator[] (const char *key) const LIFETIMEBOUND    {return asDict().get(key);}
+        Value operator[] (const KeyPath &kp) const LIFETIMEBOUND  {return root().operator[](kp);}
 
         bool operator== (const Doc &d) const        {return _doc == d._doc;}
 
@@ -415,7 +412,7 @@ namespace fleece {
         explicit Encoder(FLSharedKeys FL_NULLABLE sk)   :Encoder() {setSharedKeys(sk);}
 
         explicit Encoder(FLEncoder enc)                 :_enc(enc) { }
-        Encoder(Encoder&& enc)                          :_enc(enc._enc) {enc._enc = nullptr;}
+        Encoder(Encoder&& enc) noexcept                 :_enc(enc._enc) {enc._enc = nullptr;}
 
         void detach()                                   {_enc = nullptr;}
 
@@ -434,7 +431,7 @@ namespace fleece {
         inline bool writeDouble(double);
         inline bool writeString(slice);
         inline bool writeString(const char *s)          {return writeString(slice(s));}
-        inline bool writeString(std::string s)          {return writeString(slice(s));}
+        inline bool writeString(const std::string& s)   {return writeString(slice(s));}
         inline bool writeDateString(FLTimestamp, bool asUTC =true);
         inline bool writeData(slice);
         inline bool writeValue(Value);
@@ -537,6 +534,10 @@ namespace fleece {
 
     //====== IMPLEMENTATION GUNK:
 
+    static_assert(std::is_trivially_copyable_v<Value>);
+    static_assert(std::is_trivially_copyable_v<Array>);
+    static_assert(std::is_trivially_copyable_v<Dict>);
+
     inline FLValueType Value::type() const      {return FLValue_GetType(_val);}
     inline bool Value::isInteger() const        {return FLValue_IsInteger(_val);}
     inline bool Value::isUnsigned() const       {return FLValue_IsUnsigned(_val);}

diff --git a/API/fleece/PlatformCompat.hh b/API/fleece/PlatformCompat.hh
@@ -12,15 +12,12 @@
 
 #pragma once
 #include "fleece/CompilerSupport.h"
-#ifdef __APPLE__
-    #include <sys/cdefs.h>
-    #include "TargetConditionals.h"
-#endif
 
 #ifdef _MSC_VER
     #define NOINLINE                        __declspec(noinline)
     #define ALWAYS_INLINE                   inline
     #define ASSUME(cond)                    __assume(cond)
+    #define __typeof                        decltype
 
     #define __func__                        __FUNCTION__
 

diff --git a/API/fleece/RefCounted.hh b/API/fleece/RefCounted.hh
@@ -12,7 +12,6 @@
 
 #pragma once
 #include "fleece/PlatformCompat.hh"
-#include <algorithm>
 #include <atomic>
 #include <utility>
 
@@ -108,9 +107,9 @@ namespace fleece {
 
         ~Retained()                                     {release(_ref);}
 
-        operator T* () const & noexcept FLPURE STEPOVER {return _ref;}
-        T* operator-> () const noexcept FLPURE STEPOVER {return _ref;}
-        T* get() const noexcept FLPURE STEPOVER         {return _ref;}
+        operator T* () const & noexcept LIFETIMEBOUND FLPURE STEPOVER {return _ref;}
+        T* operator-> () const noexcept LIFETIMEBOUND FLPURE STEPOVER {return _ref;}
+        T* get() const noexcept LIFETIMEBOUND FLPURE STEPOVER         {return _ref;}
 
         explicit operator bool () const FLPURE          {return (_ref != nullptr);}
 
@@ -150,6 +149,11 @@ namespace fleece {
         [[nodiscard]]
         T* detach() && noexcept                  {auto r = _ref; _ref = nullptr; return r;}
 
+        /// Equivalent to `get` but without the `LIFETIMEBOUND` attribute. For use in rare cases where you have
+        /// a `Retained<T>` and need to return it as a `T*`, which is normally illegal, but you know that there's
+        /// another `Retained` value keeping the object alive even after this function returns.
+        T* unsafe_get() const noexcept          {return _ref;}
+
         // The operator below is often a dangerous mistake, so it's deliberately made impossible.
         // It happens in these sorts of contexts, where it can produce a dangling pointer to a
         // deleted object:
@@ -198,9 +202,9 @@ namespace fleece {
         RetainedConst(Retained<T> &&r) noexcept         :_ref(std::move(r).detach()) { }
         ALWAYS_INLINE ~RetainedConst()                  {release(_ref);}
 
-        operator const T* () const & noexcept FLPURE STEPOVER   {return _ref;}
-        const T* operator-> () const noexcept FLPURE STEPOVER   {return _ref;}
-        const T* get() const noexcept FLPURE STEPOVER           {return _ref;}
+        operator const T* () const & noexcept LIFETIMEBOUND FLPURE STEPOVER   {return _ref;}
+        const T* operator-> () const noexcept LIFETIMEBOUND FLPURE STEPOVER   {return _ref;}
+        const T* get() const noexcept LIFETIMEBOUND FLPURE STEPOVER           {return _ref;}
 
         RetainedConst& operator=(const T *t) & noexcept {
             auto oldRef = _ref;