This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] modernize-loop-convert reverse iteration support
ClosedPublic

Authored by njames93 on Jun 18 2020, 6:00 AM.

Details

Summary

Enables support for transforming loops of the form

for (auto I = Cont.rbegin(), E = Cont.rend(); I != E;++I)

This is done automatically in C++20 mode using std::ranges::reverse_view but there are options to specify a different function to reverse iterator over a container.
This is the first step, down the line I'd like to possibly extend this support for array based loops

for (unsigned I = Arr.size() - 1;I >=0;--I) Arr[I]...

Currently if you pass a reversing function with no header in the options it will just assume that the function exists, however as we have the ASTContext it may be as wise to check before applying, or at least lower the confidence level if we can't find it.

Diff Detail

Unit TestsFailed

Event Timeline

njames93 created this revision.Jun 18 2020, 6:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 18 2020, 6:00 AM

It'll be interesting to run improved check over LLVM code base.

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
997

I think IsEnabled should be initialized as member or in constructor body because of non-trivial logic.

1003

Something need to be changed in: not, Disabling - comma to dot or capitalization.

clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst
123

Please separate with empty line.

141

Please highlight ranges::reverse_view and llvm::reverse with double back-ticks.

njames93 updated this revision to Diff 271723.Jun 18 2020, 8:07 AM
njames93 marked 4 inline comments as done.

Address reviewer comments.

njames93 updated this revision to Diff 271856.Jun 18 2020, 3:01 PM
  • Change default c++20 include to ranges instead of algorithm

It'll be interesting to run improved check over LLVM code base.

I haven't tried running it over llvm, but I have had success using it with clangd in my editor, every loop I've tried seems to work a charm.

njames93 updated this revision to Diff 272030.Jun 19 2020, 5:29 AM
  • Set and store the IncludeStyle
njames93 updated this revision to Diff 272034.Jun 19 2020, 5:49 AM

Add documentation about the include style optione

Harbormaster completed remote builds in B61004: Diff 272034.
njames93 updated this revision to Diff 273913.Jun 28 2020, 2:33 AM

Rebased inline with new enum handling added in D82188

njames93 updated this revision to Diff 278398.Jul 16 2020, 1:49 AM

Updated release notes for version bump.

njames93 updated this revision to Diff 282757.Aug 3 2020, 4:10 PM
  • Rebased trunk
  • Cleaned up test cases
  • Added support for specifying to include as system include.
alexfh requested changes to this revision.Aug 10 2020, 6:29 AM

Thanks for the patch! Looks generally good. A few comments inline.

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
73–74

These are used in two places close together. I'd just use literals like for the other option names.

928

/*IsReverse=*/

1010

I'd just use string literals directly.

1018–1022

It looks like this should be a feature of the IncludeInserter. Not necessarily in this patch though. The createIncludeInsertion in other checks could be made a bit more self-descriptive too, e.g. this one in clang-tidy/modernize/MakeSmartPtrCheck.cpp:

Diag << Inserter.createIncludeInsertion(
    FD, MakeSmartPtrFunctionHeader,
    /*IsAngled=*/MakeSmartPtrFunctionHeader == StdMemoryHeader);

could have angle brackets in MakeSmartPtrFunctionHeader instead of making a special case for memory.

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h
42–63

This abstraction doesn't seem to give much benefit over placing all these fields into the LoopConvertCheck class.

58

Use = false; for initialization. It's more common in LLVM.

This revision now requires changes to proceed.Aug 10 2020, 6:29 AM
alexfh added inline comments.Aug 10 2020, 10:24 AM
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
1018–1022

I've posted a patch for this for review: https://reviews.llvm.org/D85666.

njames93 updated this revision to Diff 294895.Sep 28 2020, 11:54 PM

Address reviewer comments

njames93 marked 6 inline comments as done.Sep 28 2020, 11:54 PM
njames93 updated this revision to Diff 294897.Sep 28 2020, 11:55 PM
njames93 marked an inline comment as done.

IsReverse=

alexfh accepted this revision.Oct 12 2020, 6:36 AM

Looks good!

This revision is now accepted and ready to land.Oct 12 2020, 6:36 AM
This revision was landed with ongoing or failed builds.Oct 16 2020, 6:16 AM
This revision was automatically updated to reflect the committed changes.