pw_status: Optionally add [[nodiscard]] to status
- Add a config option to pw_status that enables [[nodiscard]] to
pw::Status and pw::StatusWithSize. Default to off.
- Add [[nodiscard]] to pw::Result.
- Add Abseil-style IgnoreError() functions to Status, StatusWithSize,
and Result.
Change-Id: Iaa11ce5edc963e8bdd795e743545bafa9e42bf62
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/46603
Reviewed-by: Ewout van Bekkum <ewout@google.com>
Commit-Queue: Wyatt Hepler <hepler@google.com>
diff --git a/pw_protobuf/encoder_fuzzer.cc b/pw_protobuf/encoder_fuzzer.cc
index f22eaed..8eb7ed6 100644
--- a/pw_protobuf/encoder_fuzzer.cc
+++ b/pw_protobuf/encoder_fuzzer.cc
@@ -152,7 +152,7 @@
switch (provider.ConsumeEnum<FieldType>()) {
case kEncodeAndClear:
// Special "field". Encode all the fields so far and reset the encoder.
- encoder.Encode();
+ encoder.Encode().IgnoreError();
encoder.Clear();
break;
case kUint32:
@@ -276,7 +276,7 @@
}
}
// Ensure we call `Encode` at least once.
- encoder.Encode();
+ encoder.Encode().IgnoreError();
// Don't forget to unpoison for the next iteration!
ASAN_UNPOISON_MEMORY_REGION(poisoned, poisoned_length);
diff --git a/pw_result/public/pw_result/result.h b/pw_result/public/pw_result/result.h
index 0fcffd1..642b667 100644
--- a/pw_result/public/pw_result/result.h
+++ b/pw_result/public/pw_result/result.h
@@ -29,7 +29,7 @@
// TODO(pwbug/363): Refactor pw::Result to properly support non-default move
// and/or copy assignment operators and/or constructors.
template <typename T>
-class Result {
+class [[nodiscard]] Result {
public:
constexpr Result(T&& value) : value_(std::move(value)), status_(OkStatus()) {}
constexpr Result(const T& value) : value_(value), status_(OkStatus()) {}
@@ -51,8 +51,8 @@
constexpr Result(Result&&) = default;
constexpr Result& operator=(Result&&) = default;
- constexpr Status status() const { return status_; }
- constexpr bool ok() const { return status_.ok(); }
+ [[nodiscard]] constexpr Status status() const { return status_; }
+ [[nodiscard]] constexpr bool ok() const { return status_.ok(); }
constexpr T& value() & {
PW_ASSERT(status_.ok());
@@ -89,6 +89,11 @@
return std::forward<U>(default_value);
}
+ // Ignores any errors. This method does nothing except potentially suppress
+ // complaints from any tools that are checking that errors are not dropped on
+ // the floor.
+ constexpr void IgnoreError() const {}
+
private:
struct Dummy {};
diff --git a/pw_status/BUILD b/pw_status/BUILD
index c4089d2..170626f 100644
--- a/pw_status/BUILD
+++ b/pw_status/BUILD
@@ -24,7 +24,10 @@
pw_cc_library(
name = "pw_status",
- srcs = ["status.cc"],
+ srcs = [
+ "public/pw_status/internal/config.h",
+ "status.cc",
+ ],
hdrs = [
"public/pw_status/status.h",
"public/pw_status/status_with_size.h",
diff --git a/pw_status/BUILD.gn b/pw_status/BUILD.gn
index c1d7086..0b51fd0 100644
--- a/pw_status/BUILD.gn
+++ b/pw_status/BUILD.gn
@@ -14,22 +14,45 @@
import("//build_overrides/pigweed.gni")
+import("$dir_pw_build/module_config.gni")
import("$dir_pw_build/target_types.gni")
import("$dir_pw_docgen/docs.gni")
import("$dir_pw_unit_test/test.gni")
-config("default_config") {
+declare_args() {
+ # The build target that overrides the default configuration options for this
+ # module. This should point to a source set that provides defines through a
+ # public config (which may -include a file or add defines directly).
+ pw_status_CONFIG = pw_build_DEFAULT_MODULE_CONFIG
+}
+
+config("public_include_path") {
include_dirs = [ "public" ]
+ visibility = [ ":*" ]
}
pw_source_set("pw_status") {
- public_configs = [ ":default_config" ]
+ public_configs = [ ":public_include_path" ]
+ public_deps = [ pw_status_CONFIG ]
public = [
"public/pw_status/status.h",
"public/pw_status/status_with_size.h",
"public/pw_status/try.h",
]
- sources = [ "status.cc" ]
+ sources = [
+ "public/pw_status/internal/config.h",
+ "status.cc",
+ ]
+}
+
+config("check_if_used_config") {
+ defines = [ "PW_STATUS_CHECK_IF_USED=1" ]
+ visibility = [ ":*" ]
+}
+
+# Use this for pw_status_CONFIG to require pw::Status objects to be used.
+pw_source_set("check_if_used") {
+ public_configs = [ ":check_if_used_config" ]
}
pw_test_group("tests") {
diff --git a/pw_status/docs.rst b/pw_status/docs.rst
index c2595c5..a6434bf 100644
--- a/pw_status/docs.rst
+++ b/pw_status/docs.rst
@@ -220,6 +220,17 @@
}
return overall_status;
+Unused result warnings
+----------------------
+If the ``PW_STATUS_CHECK_IF_USED`` option is enabled, ``pw::Status`` objects
+returned from function calls must be used or it is a compilation error. To
+silence these warnings call ``IgnoreError()`` on the returned status object.
+
+``PW_STATUS_CHECK_IF_USED`` defaults to off. Pigweed and projects that use it
+will be updated to compile with this option enabled. After all projects have
+migrated, unused result warnings will be enabled unconditionally. See
+`pwbug/387 <https://bugs.chromium.org/p/pigweed/issues/detail?id=387>`_.
+
C compatibility
---------------
``pw_status`` provides the C-compatible ``pw_Status`` enum for the status codes.
diff --git a/pw_status/public/pw_status/internal/config.h b/pw_status/public/pw_status/internal/config.h
new file mode 100644
index 0000000..62db941
--- /dev/null
+++ b/pw_status/public/pw_status/internal/config.h
@@ -0,0 +1,28 @@
+// Copyright 2021 The Pigweed Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may not
+// use this file except in compliance with the License. You may obtain a copy of
+// the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+// License for the specific language governing permissions and limitations under
+// the License.
+#pragma once
+
+// Controls whether to check if pw::Status values are used. Ununsed Status
+// values cause compilation warnings / errors. Calling the nop IgnoreError()
+// function silences these warnings.
+#ifndef PW_STATUS_CFG_CHECK_IF_USED
+#define PW_STATUS_CFG_CHECK_IF_USED 0
+#endif // PW_STATUS_CFG_CHECK_IF_USED
+
+// Set internal macro that optionally adds the [[nodiscard]] attribute.
+#if PW_STATUS_CFG_CHECK_IF_USED
+#define _PW_STATUS_NO_DISCARD [[nodiscard]]
+#else
+#define _PW_STATUS_NO_DISCARD
+#endif // PW_STATUS_CFG_CHECK_IF_USED
diff --git a/pw_status/public/pw_status/status.h b/pw_status/public/pw_status/status.h
index eb02440..8ed5330 100644
--- a/pw_status/public/pw_status/status.h
+++ b/pw_status/public/pw_status/status.h
@@ -13,6 +13,8 @@
// the License.
#pragma once
+#include "pw_status/internal/config.h"
+
#ifdef __cplusplus
extern "C" {
#endif // __cplusplus
@@ -185,7 +187,7 @@
// The Status class is a thin, zero-cost abstraction around the pw_Status enum.
// It initializes to OkStatus() by default and adds ok() and str() methods.
// Implicit conversions are permitted between pw_Status and pw::Status.
-class Status {
+class _PW_STATUS_NO_DISCARD Status {
public:
using Code = pw_Status;
@@ -312,6 +314,11 @@
}
}
+ // Ignores any errors. This method does nothing except potentially suppress
+ // complaints from any tools that are checking that errors are not dropped on
+ // the floor.
+ constexpr void IgnoreError() const {}
+
// Returns a null-terminated string representation of the Status.
[[nodiscard]] const char* str() const { return pw_StatusString(code_); }
diff --git a/pw_status/public/pw_status/status_with_size.h b/pw_status/public/pw_status/status_with_size.h
index 48b1496..9dfb952 100644
--- a/pw_status/public/pw_status/status_with_size.h
+++ b/pw_status/public/pw_status/status_with_size.h
@@ -45,7 +45,7 @@
// At least for ARMv7-M, the returned struct is created on the stack, which
// increases code size.
//
-class StatusWithSize {
+class _PW_STATUS_NO_DISCARD StatusWithSize {
public:
static constexpr StatusWithSize Cancelled(size_t size = 0) {
return StatusWithSize(Status::Cancelled(), size);
@@ -114,15 +114,22 @@
constexpr StatusWithSize& operator=(const StatusWithSize&) = default;
// Returns the size. The size is always present, even if status() is an error.
- constexpr size_t size() const { return size_ & kSizeMask; }
+ [[nodiscard]] constexpr size_t size() const { return size_ & kSizeMask; }
// The maximum valid value for size.
- static constexpr size_t max_size() { return kSizeMask; }
+ [[nodiscard]] static constexpr size_t max_size() { return kSizeMask; }
// True if status() == OkStatus().
- constexpr bool ok() const { return (size_ & kStatusMask) == 0u; }
+ [[nodiscard]] constexpr bool ok() const {
+ return (size_ & kStatusMask) == 0u;
+ }
- constexpr Status status() const {
+ // Ignores any errors. This method does nothing except potentially suppress
+ // complaints from any tools that are checking that errors are not dropped on
+ // the floor.
+ constexpr void IgnoreError() const {}
+
+ [[nodiscard]] constexpr Status status() const {
return static_cast<Status::Code>((size_ & kStatusMask) >> kStatusShift);
}
