Page MenuHomePhabricator

[clang-tidy] RenamerClangTidy now renames dependent member expr when the member can be resolved
Needs ReviewPublic

Authored by njames93 on Mon, Jan 20, 8:46 AM.

Details

Summary

take this code

template<typename T>
class A{
  int value;
  A& operator=(const A& Other){
    value = Other.value;
    this->value = Other.value;
    return *this;
  }
};

Any explicit access to value is handled by the AST as a CXXDependentScopeMemberExpr. However we have enough information to be able to resolve that the explicit access is refering to int value;. The old behaviour of the RenamerClangTidy logic would be to ignore this access, This patch addresses this shortfall

Diff Detail

Event Timeline

njames93 created this revision.Mon, Jan 20, 8:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jan 20, 8:46 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
njames93 retitled this revision from First Draft to [clang-tidy] RenamerClangTidy now renames dependent member expr when the member can be resolved.Mon, Jan 20, 8:47 AM
njames93 edited the summary of this revision. (Show Details)Mon, Jan 20, 8:50 AM
njames93 updated this revision to Diff 239150.Mon, Jan 20, 8:54 AM
  • replace auto when type isnt explicit
JonasToth added inline comments.Mon, Jan 20, 8:59 AM
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
363

can Base be a const pointer?

In which case will this pointer be a nullptr? Your test-cases seem to exercise only cases where Base is not-null.

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp
180

Could you please provide a test-case with non-type template parameters as well?

Another question but maybe/most likely off topic: are how are member-pointers handled? Both for data and member-functions.
-> Maybe a test-case for such a situation would be good, as well? :)

Crème de la Crème would be if templated member-pointers are treated properly, too.

Unit tests: pass. 61955 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: pass. 61955 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Eugene.Zelenko edited reviewers, added: alexfh, hokein; removed: logan-5.Mon, Jan 20, 10:08 AM
Eugene.Zelenko added a project: Restricted Project.
njames93 updated this revision to Diff 239167.Mon, Jan 20, 10:28 AM
njames93 marked 4 inline comments as done.
  • Added not type template test case
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
363

Base can be a const pointer. However I don't think it can be a nullptr, but I put the check in there just in case it ever is, maybe an assert would be the correct solution.

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp
180

The test for non-type isn't strictly needed. in these test cases the template type isn't used but they still show up as dependent scope exprs.

MemberPointers is something that this doesn't handle and probably something I wont add as you can have issues with specialisation meaning the member may not exist in a specialization

Unit tests: pass. 62027 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

hmmm annoyingly this change causes a crash in a side project I'm creating, gotta go bug hunting now

njames93 updated this revision to Diff 239208.Mon, Jan 20, 4:35 PM
  • Added some sanity tests that fix weird crashes
  • Added support for CXXMethodDecls that are referenced with depended scope member expr

Unit tests: fail. 62026 tests passed, 1 failed and 783 were skipped.

failed: Clang Tools.clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: fail. 62026 tests passed, 1 failed and 783 were skipped.

failed: Clang Tools.clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

njames93 updated this revision to Diff 239218.Mon, Jan 20, 5:34 PM
  • rebase master figure out the llvm pre-merge lint test fail

Unit tests: fail. 62040 tests passed, 1 failed and 783 were skipped.

failed: Clang Tools.clang-tidy/checkers/readability-identifier-naming-member-decl-usage.cpp

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

njames93 updated this revision to Diff 239253.Tue, Jan 21, 1:52 AM
  • Fix assertion causing failing tests in debug

Unit tests: pass. 62041 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

njames93 updated this revision to Diff 241449.Thu, Jan 30, 7:22 AM
  • Aggressive dependent lookup option added

Unit tests: pass. 62329 tests passed, 0 failed and 838 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

njames93 updated this revision to Diff 241459.Thu, Jan 30, 7:54 AM
  • Fix clang tidy warning

Unit tests: pass. 62329 tests passed, 0 failed and 838 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

njames93 updated this revision to Diff 241515.Thu, Jan 30, 10:53 AM
  • Streamline memberExpr matchers

Unit tests: fail. 62347 tests passed, 1 failed and 839 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

I toyed with the idea of checking the access specifier for the dependent base classes, but I'm pretty sure it doesn't matter. If the compile finds 2 look ups in one base class it will pick the later dependancy regardless of whether its private or not. And if the compiler finds 2 look ups in multiple inheritance bases it will always be an error, regardless of access specifier

njames93 updated this revision to Diff 243953.Tue, Feb 11, 12:23 PM
  • Simplified member expr restrictions
njames93 updated this revision to Diff 244087.Wed, Feb 12, 12:51 AM
  • Made AggressiveDependentMemberLookup option Global
njames93 updated this revision to Diff 244601.Fri, Feb 14, 2:58 AM
  • Small refactor of code