Mark fallible container operations as `nodiscard`
It's evidently too easy to ignore the return values of these operations.
After all, ignoring allocation failures is the normal pattern in most
C++ code.
Thus this change marks these functions as `nodiscard`. This does require
a few CHECKs in test code, but it also catches one real instance of a
problem.
Change-Id: I24432506283145fc2f459336fe1035cbca27bd4f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/75187
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/ssl/handoff.cc b/ssl/handoff.cc
index bd409f2..610ad6f 100644
--- a/ssl/handoff.cc
+++ b/ssl/handoff.cc
@@ -668,8 +668,10 @@
}
s3->session_reused = session_reused;
hs->channel_id_negotiated = channel_id_negotiated;
- s3->next_proto_negotiated.CopyFrom(next_proto);
- s3->alpn_selected.CopyFrom(alpn);
+ if (!s3->next_proto_negotiated.CopyFrom(next_proto) ||
+ !s3->alpn_selected.CopyFrom(alpn)) {
+ return false;
+ }
const size_t hostname_len = CBS_len(&hostname);
if (hostname_len == 0) {
diff --git a/ssl/internal.h b/ssl/internal.h
index 0f3da92..bf8bf36 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -161,7 +161,7 @@
// value-constructed copies of |T|. It returns true on success and false on
// error. If |T| is a primitive type like |uint8_t|, value-construction means
// it will be zero-initialized.
- bool Init(size_t new_size) {
+ [[nodiscard]] bool Init(size_t new_size) {
if (!InitUninitialized(new_size)) {
return false;
}
@@ -172,7 +172,7 @@
// InitForOverwrite behaves like |Init| but it default-constructs each element
// instead. This means that, if |T| is a primitive type, the array will be
// uninitialized and thus must be filled in by the caller.
- bool InitForOverwrite(size_t new_size) {
+ [[nodiscard]] bool InitForOverwrite(size_t new_size) {
if (!InitUninitialized(new_size)) {
return false;
}
@@ -182,7 +182,7 @@
// CopyFrom replaces the array with a newly-allocated copy of |in|. It returns
// true on success and false on error.
- bool CopyFrom(Span<const T> in) {
+ [[nodiscard]] bool CopyFrom(Span<const T> in) {
if (!InitUninitialized(in.size())) {
return false;
}
@@ -273,7 +273,7 @@
// Push adds |elem| at the end of the internal array, growing if necessary. It
// returns false when allocation fails.
- bool Push(T elem) {
+ [[nodiscard]] bool Push(T elem) {
if (!MaybeGrow()) {
return false;
}
@@ -284,7 +284,7 @@
// CopyFrom replaces the contents of the array with a copy of |in|. It returns
// true on success and false on allocation error.
- bool CopyFrom(Span<const T> in) {
+ [[nodiscard]] bool CopyFrom(Span<const T> in) {
Array<T> copy;
if (!copy.CopyFrom(in)) {
return false;
@@ -406,7 +406,7 @@
// TryResize resizes the vector to |new_size| and returns true, or returns
// false if |new_size| is too large. Any newly-added elements are
// value-initialized.
- bool TryResize(size_t new_size) {
+ [[nodiscard]] bool TryResize(size_t new_size) {
if (new_size <= size_) {
Shrink(new_size);
return true;
@@ -422,7 +422,7 @@
// TryResizeForOverwrite behaves like |TryResize|, but newly-added elements
// are default-initialized, so POD types may contain uninitialized values that
// the caller is responsible for filling in.
- bool TryResizeForOverwrite(size_t new_size) {
+ [[nodiscard]] bool TryResizeForOverwrite(size_t new_size) {
if (new_size <= size_) {
Shrink(new_size);
return true;
@@ -437,7 +437,7 @@
// TryCopyFrom sets the vector to a copy of |in| and returns true, or returns
// false if |in| is too large.
- bool TryCopyFrom(Span<const T> in) {
+ [[nodiscard]] bool TryCopyFrom(Span<const T> in) {
if (in.size() > capacity()) {
return false;
}
@@ -449,7 +449,7 @@
// TryPushBack appends |val| to the vector and returns a pointer to the
// newly-inserted value, or nullptr if the vector is at capacity.
- T *TryPushBack(T val) {
+ [[nodiscard]] T *TryPushBack(T val) {
if (size() >= capacity()) {
return nullptr;
}
diff --git a/ssl/ssl_internal_test.cc b/ssl/ssl_internal_test.cc
index 230018f..ae251dc 100644
--- a/ssl/ssl_internal_test.cc
+++ b/ssl/ssl_internal_test.cc
@@ -120,15 +120,15 @@
TEST(VectorTest, NotDefaultConstructible) {
struct NotDefaultConstructible {
- explicit NotDefaultConstructible(size_t n) { array.Init(n); }
+ explicit NotDefaultConstructible(size_t n) { BSSL_CHECK(array.Init(n)); }
Array<int> array;
};
Vector<NotDefaultConstructible> vec;
- vec.Push(NotDefaultConstructible(0));
- vec.Push(NotDefaultConstructible(1));
- vec.Push(NotDefaultConstructible(2));
- vec.Push(NotDefaultConstructible(3));
+ ASSERT_TRUE(vec.Push(NotDefaultConstructible(0)));
+ ASSERT_TRUE(vec.Push(NotDefaultConstructible(1)));
+ ASSERT_TRUE(vec.Push(NotDefaultConstructible(2)));
+ ASSERT_TRUE(vec.Push(NotDefaultConstructible(3)));
EXPECT_EQ(vec.size(), 4u);
EXPECT_EQ(0u, vec[0].array.size());
EXPECT_EQ(1u, vec[1].array.size());
