Page MenuHomePhabricator

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

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

Details

Summary

Sometimes in templated code Member references are reported as DependentScopeMemberExpr because that's what the standard dictates, however in many trivial cases it is easy to resolve the reference to its actual Member.
Take this code:

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

When ran with clang-tidy file.cpp -checks=readability-identifier-naming --config="{CheckOptions: [{key: readability-identifier-naming.MemberPrefix, value: m_}]}" -fix
Current behaviour:

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

As this->value and Other.value are Dependent they are ignored when creating the fix-its, however this can easily be resolved.
Proposed behaviour:

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

Diff Detail

Event Timeline

njames93 created this revision.Jan 20 2020, 8:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2020, 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.Jan 20 2020, 8:47 AM
njames93 edited the summary of this revision. (Show Details)Jan 20 2020, 8:50 AM
njames93 updated this revision to Diff 239150.Jan 20 2020, 8:54 AM
  • replace auto when type isnt explicit
JonasToth added inline comments.Jan 20 2020, 8:59 AM
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
351

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.Jan 20 2020, 10:08 AM
Eugene.Zelenko added a project: Restricted Project.
njames93 updated this revision to Diff 239167.Jan 20 2020, 10:28 AM
njames93 marked 4 inline comments as done.
  • Added not type template test case
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
351

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.Jan 20 2020, 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.Jan 20 2020, 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.Jan 21 2020, 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.Jan 30 2020, 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.Jan 30 2020, 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.Jan 30 2020, 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.Feb 11 2020, 12:23 PM
  • Simplified member expr restrictions
njames93 updated this revision to Diff 244087.Feb 12 2020, 12:51 AM
  • Made AggressiveDependentMemberLookup option Global
njames93 updated this revision to Diff 244601.Feb 14 2020, 2:58 AM
  • Small refactor of code
njames93 updated this revision to Diff 251231.Mar 18 2020, 5:29 PM
  • rebase and small nit
njames93 edited the summary of this revision. (Show Details)Mar 19 2020, 1:46 PM
njames93 updated this revision to Diff 257151.Apr 13 2020, 4:44 PM

Clean up code and remove unrelated changes

aaron.ballman added inline comments.Apr 27 2020, 2:01 PM
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
185–187

Make these explicit?

clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
43

I'd appreciate some comments here explaining when this should be overridden. I'd also like to understand why we need storeOptions and storeCheckOptions because the two names are so similar to one another.

njames93 marked an inline comment as done.May 7 2020, 5:35 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
43

I'm still unsure what the best approach here is. RenamerClangTidyCheck::storeOptions needs to be called to store the AggressiveDependentMemberLookup Option (as well as any other renaming specific option that may be added down the line). However derived classes need to also store their respective options at the same time. Due to how c++ works there isn't a nice way to ensure base methods are called at the same time as derived methods.
Anyway that's the reason the names are similar they do a very similar job only one is meant for the RenamerClangTidyCheck class and the other for derived classes.
I agree a comment is needed to explain why so I'll add that in for now

njames93 updated this revision to Diff 262865.May 8 2020, 6:44 AM
  • Address nits
aaron.ballman added inline comments.May 8 2020, 8:18 AM
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
43

I think it's reasonable to expect developers to call Base::storeOptions() from within Derived::storeOptions() and we don't need a second method to override. You're correct that there's not a good way to ensure the base method is called, but test cases should exist for the options in a check and so I would imagine that a developer who forgets to call the base class function will have failing test cases to help alert them to the issue. WDYT?

njames93 updated this revision to Diff 262920.May 8 2020, 11:30 AM
njames93 marked 3 inline comments as done.
  • Tweaked options to remove second virtual method
aaron.ballman accepted this revision.May 8 2020, 12:04 PM

LGTM, thank you for this!

This revision is now accepted and ready to land.May 8 2020, 12:04 PM
This revision was automatically updated to reflect the committed changes.