map::operator[] should be nodiscard
map::operator[] should be nodiscardLately 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 std::min, 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
p.release();
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
(void)p.release();
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
mymap[key];
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()];
// 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++ respnded 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 tom.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:
- “Should
std::expectedbe nodiscard?” (2024-12-08) — Yes, it should.
