Page MenuHomePhabricator

[clang-tidy] Remove gsl::at suggestion from cppcoreguidelines-pro-bounds-constant-array-index
ClosedPublic

Authored by carlosgalvezp on Jan 21 2022, 12:15 AM.

Details

Summary

Currently the fix hint is hardcoded to gsl::at(). This poses
a problem for people who, for a number of reasons, don't want
or cannot use the GSL library (introducing a new third-party
dependency into a project is not a minor task).

In these situations, the fix hint does more harm than good
as it creates confusion as to what the fix should be. People
can even misinterpret the fix "gsl::at" as e.g. "std::array::at",
which can lead to even more trouble (e.g. when having guidelines
that disallow exceptions).

Furthermore, this is not a requirement from the C++ Core Guidelines.
simply that array indexing needs to be safe. Each project should
be able to decide upon a strategy for safe indexing.

The fix-it is kept for people who want to use the GSL library.

Diff Detail

Event Timeline

carlosgalvezp created this revision.Jan 21 2022, 12:15 AM
carlosgalvezp requested review of this revision.Jan 21 2022, 12:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2022, 12:15 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Fix alphabetical order in release notes.

carlosgalvezp edited the summary of this revision. (Show Details)

Update commit message.

salman-javed-nz accepted this revision.Jan 23 2022, 5:30 AM

LGTM besides a couple of nits. I think the diagnostic message should just say what the problem is, not what the fix is - that's what the FixitHint is for.

clang-tools-extra/docs/ReleaseNotes.rst
163
164

To match "C++ Core Guidelines" in the previous bullet point.

This revision is now accepted and ready to land.Jan 23 2022, 5:30 AM
carlosgalvezp marked 2 inline comments as done.
carlosgalvezp edited the summary of this revision. (Show Details)

Fixed review comments

This revision was landed with ongoing or failed builds.Jan 23 2022, 8:07 AM
This revision was automatically updated to reflect the committed changes.