This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] add check cppcoreguidelines-pro-bounds-constant-array-index
ClosedPublic

Authored by mgehre on Oct 14 2015, 2:57 PM.

Details

Summary

This check flags all array subscriptions on static arrays and
std::arrays that either have a non-compile-time-constant index or are
out of bounds.

Dynamic accesses into arrays are difficult for both tools and humans to
validate as safe. array_view is a bounds-checked, safe type for
accessing arrays of data. at() is another alternative that ensures
single accesses are bounds-checked. If iterators are needed to access an
array, use the iterators from an array_view constructed over the array.

This rule is part of the "Bounds safety" profile of the C++ Core
Guidelines, see
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-bounds2-only-index-into-arrays-using-constant-expressions

Diff Detail

Event Timeline

mgehre updated this revision to Diff 37395.Oct 14 2015, 2:57 PM
mgehre retitled this revision from to [clang-tidy] add check cppcoreguidelines-pro-bounds-constant-array-index.
mgehre updated this object.
mgehre added a subscriber: cfe-commits.
mgehre updated this revision to Diff 37401.Oct 14 2015, 3:18 PM

Remove dead codet

sbenza edited edge metadata.Oct 16 2015, 10:32 AM

Thanks for the patch.
A few comments below.

clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
23

All this code needs to be reformatted to 80 cols.
http://llvm.org/docs/CodingStandards.html#source-code-width

Have you tried clang-format?
It does a great job here.

28
hasType(qualType(hasDeclaration(classTemplateSpecializationDecl(hasName("::std::array"))))

can be written as

hasType(cxxRecordDecl(hasName("::std::array")))
36

Move this declaration closer to its first use.

55

Space after //

55

If this check is enabled, it is very likely that the library is available.
Maybe we should suggest the fix.
You can use clang::tidy::IncludeInserter to add the include if needed.

We could also add a configuration option to enable/disable the automated fix for this case.

clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.h
18

reflow to 80 cols

mgehre added inline comments.Oct 16 2015, 12:16 PM
clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
55

One issue I see is that the gsl header does not have a standard name. I currently know of the microsoft gsl and gsl-lite (https://github.com/martinmoene/gsl-lite), and they don't agree on the name of the header.

I could make an option "gsl-header-name", which defaults to an empty string. If it is set to an non-empty filename,
the fix is enabled and that header is used.

What do you think?

mgehre updated this revision to Diff 37627.Oct 16 2015, 12:40 PM
mgehre marked 5 inline comments as done.
mgehre edited edge metadata.

Thanks for the comments!

Fixed most comments in updated revision

sbenza added inline comments.Oct 16 2015, 1:46 PM
clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
55

Interesting.
The only downside I see with that is gsl-header-name might be useful for other checks in this package, and the options API only supports per-check options.
You would have to pass it once for every check that needs it.
They are specified as <CheckName>.<OptionName>.
Maybe we can add a Global category or package level flags so that it can apply to all checks under cppcoreguidelines-*.

alexfh added inline comments.Nov 5 2015, 2:13 PM
clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
57

We can go for a local setting for now, and if we introduce global or module-wide options, we can clean up these local settings in a single step.

test/clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index.cpp
51

What's the difference between this warning and the -Warray-bounds compiler diagnostic that is turned on by default?

$ cat /tmp/q.cc 
void f() {
  int a[7];
  a[10];
}
$ clang_check /tmp/q.cc --
...
/tmp/q.cc:3:3: warning: array index 10 is past the end of the array (which contains 7 elements) [-Warray-bounds]
  a[10];
  ^ ~~
/tmp/q.cc:2:3: note: array 'a' declared here
  int a[7];
  ^
mgehre updated this revision to Diff 39601.Nov 6 2015, 3:12 PM

Add option GslHeader, generate fixes

alexfh edited edge metadata.Nov 11 2015, 6:27 AM

Sorry for the delay. A few more comments.

clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
87

nit: What is true here? Please add an argument comment.

103

I'm not sure initializer lists are supported by all toolchains this code needs to compile on. Please explicitly specify SourceRange here.

125

nit: please add trailing period.

docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.rst
13

nit: please add trailing period.

test/clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index.cpp
52

So what's the difference between this warning and -Warray-bounds?

mgehre updated this revision to Diff 39952.Nov 11 2015, 11:42 AM
mgehre edited edge metadata.

update for review comments; removed bounds check on static arrays in favor of clang-diagnostic-array-bounds

mgehre updated this revision to Diff 39953.Nov 11 2015, 11:44 AM

simplify code

alexfh accepted this revision.Nov 12 2015, 6:07 AM
alexfh edited edge metadata.

Looks good with one comment.

Thank you!

clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
108

To avoid confusion, I suggest explicitly stating that this is an std::array, not a regular array ("std::array<> index %0 is before ..."). Same for the message below.

Also, it's fine to have this in clang-tidy for now, but ultimately, it may be reasonable and more consistent to analyze this in Clang as a part of -Warray-bounds. However, I'm not sure whether it's better to make a library-specific warning in Clang or introduce some annotation to tell the compiler that it can statically check indexes using a certain expression, for example.

This revision is now accepted and ready to land.Nov 12 2015, 6:07 AM
This revision was automatically updated to reflect the committed changes.

I have reverted in r253428, since I can reproduce failure locally. I'll investigate later, too.

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index.cpp
2 ↗(On Diff #40449)

Standard headers might be unavailable depending on a target, like host=linux target=msvc.
Could you improve it?

  1. Avoid <array>
  2. Introduce a new feature like "native"
mgehre added inline comments.Nov 18 2015, 1:46 PM
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index.cpp
2 ↗(On Diff #40449)

Just to make sure that I understand this: The array header is unavailble because the target is a pre-C++11 msvc?

I can replace #include <array> by an stub definition of std::array. I'm not sure what you mean by 'Introduce a new feature like "native"'?

chapuni added inline comments.Nov 18 2015, 3:59 PM
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-pro-bounds-constant-array-index.cpp
2 ↗(On Diff #40449)

It'd be good that stub version of <array>.

"Features" mean like

REQUIRES: foo-supported-feature

See also clang-tools-extra/test/lit.cfg, or clang/test/lit.cfg.