Page MenuHomePhabricator

[clang-tidy] cppcoreguidelines-declare-loop-variable-in-the-initializer: a new check
Needs ReviewPublic

Authored by DNS320 on Apr 8 2021, 1:03 AM.

Details

Summary

This check implements the ES.74 rule of the C++ Core Guidelines.

The goal of this check is to find for-statements which do not declare their loop variable in the initializer part.
This limits the loop variables visibility to the scope of the loop and prevents using the loop variable for other purposes after the loop.

Thank you for the review.

Diff Detail

Event Timeline

DNS320 created this revision.Apr 8 2021, 1:03 AM
DNS320 requested review of this revision.Apr 8 2021, 1:03 AM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.cpp
29

It'll be reasonable to add add note with actual variable declaration.

31

I think should be for statement.

clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.h
2

I may be mistaken, but proper code is -*- C++ -*-. Please remove dash at left side.

25

Please add isLanguageVersionSupported. Older versions of C doesn't support variable declarations inside for loops.

clang-tools-extra/docs/ReleaseNotes.rst
95

I think should be for statement and for highlighted with double back-ticks.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-declare-loop-variable-in-the-initializer.rst
7

Please synchronize with statement in Release Notes.

9

Such references are usually placed at the end. See other checks documentation as example.

I'd argue this check isn't following the enforcement specified in the relevant guideline:

Enforcement:
Warn when a variable modified inside the for-statement is declared outside the loop and not being used outside the loop.

All this is checking is that there is a declStmt in the for stmt, it doesn't even have to be a relevant declaration, this code would be considered ok by the implementation in here.

int I = 0;
for (int Unused = 0; I < 10; ++I) {}

Ideally you want to check for variables modified in the condition or iteration expression that aren't declared in the init statement.
To be fully compliant with the wording you would also need to ensure the variable isn't referenced outside the loop.

njames93 requested changes to this revision.Apr 9 2021, 3:50 AM
This revision now requires changes to proceed.Apr 9 2021, 3:50 AM
DNS320 added a comment.Apr 9 2021, 9:35 AM

Thank you for your reviews.
I will work on your comments and write back soon.

DNS320 updated this revision to Diff 341492.Thu, Apr 29, 6:55 AM
DNS320 marked 7 inline comments as done.

I updated the check according to the last review.
The check should now fully implement the defined enforcement chapter from the ES.74 C++ Core Guideline.

About searching for declRefExpr outside the for statement:
I limited the search to the next ancestor compoundStmt of the varDecl, so the Visitor must not traverse the whole translation unit.

DNS320 added inline comments.Thu, Apr 29, 6:57 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.cpp
29

The hole check was refactored. A diagnostic hint points now to the statement which modifies the variable.

clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.h
2

Thanks. I checked other header files with long names. I correct it this way, to match the other ones:

//===--- DeclareLoopVariableInTheInitializerCheck.h - clang-tidy-*- C++ -*-===//
//===--- ProBoundsArrayToPointerDecayCheck.h - clang-tidy--------*- C++ -*-===//
Eugene.Zelenko added inline comments.Thu, Apr 29, 7:07 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.cpp
38

const auto* could be used because type is spelled in same statement.

steveire added a subscriber: steveire.EditedThu, Apr 29, 1:50 PM

I implemented something like this recently too. The code I was trying to refactor was something like

auto ii = 0;
for (ii = 0; ii < thing.size(); ++ii)
{
    // code
}
// code
for (ii = 0; ii < thing.size(); ++ii)
{
    // code
}
// code
for (ii = 0; ii < thing.size(); ++ii)
{
    // code
}

ie, the variable was used repeatedly, but always initialized in the loop.

I also had code like

auto ii = 0;
for (ii = 0; ii < thing.size(); ++ii)
{
    // code
}
// code
ii = getNumber();
doSomething(ii);

ie, the next statement referring to ii outside the loop was an assignment, so I could just change it to

for (auto ii = 0; ii < thing.size(); ++ii)
{
    // code
}
// code
auto ii = getNumber();
doSomething(ii);

You don't necessarily have to handle these cases in the initial version of this check, but you could consider them in the future.

DNS320 updated this revision to Diff 341784.Fri, Apr 30, 12:03 AM

Renamed the IsInsideMatchedForStmt() function according to a comment from the build system.
Changed a data type to "auto" according to a comment from Eugene.Zelenko.

DNS320 marked an inline comment as done.EditedFri, Apr 30, 7:13 AM

I implemented something like this recently too. The code I was trying to refactor was something like

auto ii = 0;
for (ii = 0; ii < thing.size(); ++ii)
{
    // code
}
// code
for (ii = 0; ii < thing.size(); ++ii)
{
    // code
}
// code
for (ii = 0; ii < thing.size(); ++ii)
{
    // code
}

ie, the variable was used repeatedly, but always initialized in the loop.

I also had code like

auto ii = 0;
for (ii = 0; ii < thing.size(); ++ii)
{
    // code
}
// code
ii = getNumber();
doSomething(ii);

ie, the next statement referring to ii outside the loop was an assignment, so I could just change it to

for (auto ii = 0; ii < thing.size(); ++ii)
{
    // code
}
// code
auto ii = getNumber();
doSomething(ii);

You don't necessarily have to handle these cases in the initial version of this check, but you could consider them in the future.

Thank you for your comment!
I agree with you and I can see the need to cover such code with a style check.
The initial check was contributed to implement the ES.74 C++ Core Guideline. As I think, your code examples should be covered by a check that implements the ES.26 rule of the C++ Core Guidelines, which could be implemented in the future.
Due to your comment, I noticed the link to the ES.26 rule in the description of this check is not accurate, so I will remove it.

clang-tools-extra/clang-tidy/cppcoreguidelines/DeclareLoopVariableInTheInitializerCheck.cpp
38

Thanks!

DNS320 marked an inline comment as done.Fri, Apr 30, 7:13 AM
DNS320 updated this revision to Diff 341929.Fri, Apr 30, 8:50 AM

Removed a link to the ES.26 C++ Core Guideline in the documentation part.