Lately libc++ has been adding the C++17 [[nodiscard]] attribute aggressively to every header. (I’m not sure why this month, but my guess is that libc++ just dropped support for some old compiler such that all their supported compilers now permit the attribute even in C++11 mode.) libc++ is following the trail that Microsoft STL has blazed since VS 15.6 in late 2017.

Some functions, like malloc, always make sense to mark [[nodiscard]]. Others, like unique_ptr::release, are deliberately left unmarked because even though it is usually a bug to write

sometimes that’s actually what you meant — you’re calling it only for its side effect. Back in 2022, Stephan T. Lavavej estimated of unique_ptr::release that “90% of discards are a bug, but 10% are maybe valid… Even though it would find bugs, the cost in false positives is too great.” However, I think it’s worth pointing out that the fix for a false positive is trivial: all you have to do is refactor that line of code into

and the warning goes away. So personally I’d apply [[nodiscard]] even to unique_ptr::release. But it would clearly create noise to apply [[nodiscard]] to, for example, printf (which returns int) or vector::erase (which returns an iterator).

An interesting borderline case came up this week. In llvm-project#169971, Hristo Hristov marked libc++’s map::operator[] as nodiscard. The assumption was that it is usually a bug to write

Hans Wennborg quickly reported that actually Google’s codebases do this a fair bit. Statements calling map::operator[] purely for its side-effect are found in Chromium:

// Map the result id to the empty set.
combinator_ops_[extension->result_id()];

in V8:

// We need to insert the load into the truncation mapping as a key, because
// all loads need to be revisited during processing.
int32_truncated_loads_[op_idx];

and even in flatbuffers:

if (attributes.Add(kv->key()->str(), value)) {
  delete value;
  return false;
}
parser.known_attributes_[kv->key()->str()];

This last example doesn’t even come with a comment to explain it. If I owned this code, I’d absolutely worry that some coworker would come along and refactor it to

parser.known_attributes_[kv->key()->str()] = false;

But that would be incorrect! Assignment-of-false is an unconditional assignment; but what we have above is a conditional assignment: “If this key isn’t yet in the map, then assign it false; if it is already in the map, leave its value alone.” That is, it’s the verb that the STL calls .try_emplace, and as a maintainer I’d insist that that line be rewritten for clarity into

parser.known_attributes_.try_emplace(kv->key()->str(), false);

However, if that’s not possible (maybe because the codebase needs to keep working pre-C++17), then we can at least add the cast-to-void:

// Map to "false" only if the attribute isn't already mapped
(void)parser.known_attributes_[kv->key()->str()];

Actually, the GitHub history records that the code originally had a very clear (albeit slightly inefficient) if statement:

if (known_attributes_.find(attribute->key()->str()) == known_attributes_.end())
  known_attributes_[attribute->key()->str()] = false;

which was replaced with the current inscrutable version during review.

Anyway, libc++ responded by removing [[nodiscard]] from map::operator[] again in #172444. I think they should have stuck to their guns: mymap[key]; is a horrible way to write mymap.try_emplace(key) and STL vendors shouldn’t condone it.

Microsoft STL doesn’t mark map::operator[] as nodiscard, either, presumably because they encountered this “idiom” in the wild too. Microsoft (and now libc++) do mark the side-effect-less operator[]s of array, deque, and vector.

LLVM internally doesn’t mark the operator[]s of llvm::DenseMap, llvm::MapVector, or llvm::StringMap, although my experimentation shows that they could do so with only a handful of minor fixups (here, here, here, here), in many cases by using the more expressive and efficient tryEmplace they’ve already implemented and used as a building block for their operator[].

PSA: If your codebase contains any instances of the m[k]; “idiom,” please change them to m.try_emplace(k); or to (void)m[k]; with a code comment. Your code reviewers will thank you, and so will your library vendors!

See also: