Source
r/cpp•
Full-Spectral

match/wcmatch needs an at()

It would ever so nice if the regex match result had an alternative at() to use instead of the index operator. Some linting/analysis tools assume that this is an unchecked array operation and complain about it. This requires either lots of suppressions, or a lot of bogus warnings that hide real indexing operations that might be an issue.

It would be trivial to provide an at() that just turned around called the operator. Those who have this concern could avoid it and there would approximately zero cost to the implementors.

Posted Aug 25, 2020, 4:22 PM UTC

227
Supadoplex•

This requires either lots of suppressions

Or fixing of poorly implemented linters.

But yeah, adding at wouldn't be a bad thing in my opinion.

14
Full-SpectralOP•

Do we really require them to know whether a [] operation is range checked or not? They aren't in the standard containers, so just because it's an overridden operator doesn't mean it's safe.

1
HKei•

Given that anyone can write a [] operator it's the linters fault for not checking what type the thing [] is being called on is. Yes, it's a poorly written linter that produces false positives based on bad assumptions.

14
Full-SpectralOP•

But it cannot possibly know if it's safe or not, so it has to assume the worst. As I pointed out, the standard containers (one of the biggest uses of the index operator in a lot of code bases probably) are explicitly not range checked.

1
HKei•

The linter cannot possibly know that any operation is safe. The default is unsafe. It should therefore only warn about functions it knows for a fact are unsafe where a safe alternative exists.

Again, this is a linter problem, not an API problem.

10
Supadoplex•

Do we really require them to know whether a [] operation is range checked or not?

If the tool is going to issue a warning, then we should require them to know that there is cause to be warned, yes.

They aren't in the standard containers

Thus it may be reasonable for a tool to warn about unchecked access in that case. But that is not the case here, and the diagnostic is a false positive.

What might be a good addition to the standard are function attributes [[checked]] and [[unchecked]] - maybe [[checked(bool)]] - which could be used to by tools to make assumptions about safety of those functions. A problem with this is that safety is a rather abstract and subjective concept and this may lead to misunderstandings in more complex cases.

12
Full-SpectralOP•

There are quite a few annotations that could (and should) be added to the language to make analysis tools vastly more effective. I've argued for this a lot in the past.

1
AlexAlabuzhev•

Yes. "More warnings" != "better tool".

[] is unchecked in std containers - ok then, warn about std containers. It's not rocket science to compare the class name with "vector".

If there are both [] and at() in a user class - ok then, warn about it, "because the convention". It's not rocket science to check the class definition.

But if the analyzer's logic is simply "all [] operators are unconditionally unholy" - it's not an analyzer, it's rubbish. Even if it's Microsoft's.

8
Full-SpectralOP•

Sigh... The point is not unconditionally unholy, the point is 'potentially a problem', which it often is because index errors are common sources of bad things happening. You want it to warn you about them, but it's useless if it's lost in noise from things that we know aren't a problem but it doesn't. And putting in a bunch of special cases is not really optimal either. As others have pointed out, this does nothing for non-std stuff.

1
AlexAlabuzhev•

It's C++. Everything is "potentially a problem", including integer addition and division.

index errors are common sources of bad things happening

Used to be. I've only seen a few during the last 10 years, presumably due to the industry shift to iterators & ranges, but YMMV or course.

this does nothing for non-std stuff

As I've already mentioned above, a tool can warn if a non-std thing has both [] and at(), and it would make sense. But what's the point to complain about [] if there are no alternatives?

10
adlbd•

Are these linters advocating using at() instead of operator[] for containers like std::vector? If that's the case, you really have to question, if not the tool's author, at least the rule authors. Do they warn on all narrow-contract functions?

8
nyamatongwe•

One place this comes from is the C++ Core Guidelines and its implementation in Visual C++.

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#probounds-bounds-safety-profile

https://docs.microsoft.com/en-us/cpp/code-quality/c26446?f1url=https%3A%2F%2Fmsdn.microsoft.com%2Fquery%2Fdev16.query%3FappId%3DDev16IDEF1%26l%3DEN-US%26k%3Dk(C26446)%26rd%3Dtrue%26f%3D255%26MSPPError%3D-2147217396&view=vs-2019

3
adlbd•

That's interesting. I'm completely on board with using iterators and, with C++20, ranges but using at() just seems like poor advice. I feel less comfortable when I have to resort to indices, anyway, but it is needed at times. I've always felt that, if you don't know if your index is within the bounds, you should check it first rather than relying on exceptions to tell you what you should already know. And if you know you're within the bounds, then the bounds checking is unnecessary.

3
pjmlp•

Except when as it so frequently happens in C like code, due to copy paste errors, the bounds being compared against aren't really the actual ones.

1
Full-SpectralOP•

Or the thing being iterated through gets modified after the bounds are checked.

1
Full-SpectralOP•

They aren't advocating using .at() specifically, but it's an obvious choice. They are warning that [] is potentially a problem, and it is. On the standard containers it's unchecked, and if it's a raw index operating it is unchecked.

-1
adnukator•

It would be trivial to provide an at() that just turned around called the operator. Those who have this concern could avoid it and there would approximately zero cost to the implementors.

You can also implement a templated helper function that does the same without needing to modify the standard only because some 3rd party tooling does not work properly. You could also call *(this_match_result.begin() + offset) if you're certain that offset is less than this_match_result.size()

4
Full-SpectralOP•

Or, they could add an at(), which would be consistent with the container classes. All tooling is third party to the specification by the way, and it's Microsoft's analyzer, not exactly a fly by night operation. They can't be expected to special case every class that might implement an indexing operator.

0†
AKostur•

Here's your chance to write a paper to get it added to C++23!

Though I'm not convinced that at() is a consistent addition. at() throws an exception for out-of-bounds, and match/wcmatch has defined behaviour if you attempt to index at or beyond size(). Also note that std::set doesn't have an at() method, and it is a container class (neither does std::list).

5
Full-SpectralOP•

OK, I should have said the indexed containers (with the very inconsistent and unfortunate exception of map.) But clearly match is an indexed list of values, so it is semantically consistent with the indexed containers in that sense, hence why [] makes sense, but also why at() would as well.

Obviously analyzers could be made to special case things in the standard, but that seems a bit squishy.

1
foonathan•

FYI, the Unicode study group (read: some members of the Unicode study group have the opinion) wants to deprecate std::regex anyway, as it doesn't support unicode properly and can't, due to ABI. It is very unlikely that the committee is going to spend time on improving regex.

4
Pazer2•

All the more reason why UFCS needs to be a thing 🙃

2