[clang-tidy] Check request: readability-use-map-at

[denzor200]

Needs a check that will find an usage of std::map::find without result's checking and will suggest to use std::map::at instead.

const std::map<std::string, std::string> values = get_values();
// process(values["first"]);              // BAD - even didn't compiled
process(values.find("first")->second);    // BAD - unreadable and unsafe
process(values.at("second"));             // OK

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Denis Mikhailov (denzor200)

Needs a check that will find an usage of std::map::find without result's checking and will suggest to use std::map::at instead.

const std::map<std::string, std::string> values = get_values();
// process(values["first"]);              // BAD - even didn't compiled
process(values.find("first")->second);    // BAD - unreadable and unsafe
process(values.at("second"));             // OK

[firewave]

Using at() has the problem that you might need to add exception handling to your code and that might be undesired (potential performance implications aside).

Clean and safe modern code would look like this:

if (const auto I = values.find(""); I != values.end()) {
    process(I->second);
}

[denzor200]

potential performance implications aside

a micro performance downgrade is acceptable for a check in "readability" section I suppose.

Clean and safe modern code would look like this:

This is a serious behaviour change. Some users may never expect silence, they would prefer explicit error shown from exception.

[firewave]

a micro performance downgrade is acceptable for a check in "readability" section I suppose.

Erm, I would say "no" to that. It is just about "readability" which should not introduce any functional changes. I have not encountered it so far but from what I understand introducing exception handling could have quite an performance impact.

This is a serious behaviour change.

So is introducing the throwing of an exception. So either solution isn't right - possibly the wrong group to put it into.

The issue you are trying to fix here is about an unchecked access. That is similar to dereferencing an invalid pointer or an out-of-bounds access thus a potential bug.

[carlosgalvezp]

I think we all agree on the "unsafety" of the code, but there's disagreement on what the solution should be, which is not surprising. Everyone has their own error handling strategy: exceptions, assertions, logging, etc, etc.

So I think the check should just warn and not provide any concrete solution/fix-it, leaving it up to the user to follow the error handling guidelines applicable to their project. Then this check could be moved to bugprone instead.