This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add a new check for non-trivial unused variables.
Needs RevisionPublic

Authored by veluca93 on May 4 2022, 5:43 AM.

Details

Summary

This check is meant to detect case that -Wunused-variable does not,
by integrating some extra knowledge of types. For example, a
std::vector<int> that only ever gets push_back()-ed is similar to a
written-but-never-read variable.

Diff Detail

Event Timeline

veluca93 created this revision.May 4 2022, 5:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2022, 5:43 AM
veluca93 requested review of this revision.May 4 2022, 5:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2022, 5:43 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Eugene.Zelenko retitled this revision from Add a new clang-tidy for non-trivial unused variables. to [clang-tidy] Add a new check for non-trivial unused variables..May 4 2022, 7:14 AM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.cpp
12

C++ headers should be after application's ones.

117

const auto *. Same in other places.

clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.h
13

C++ headers should be after application's ones.

clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
9

Missing colon?

11

Missing period. Same in other places.

16

Please highlight TypesByBase with single back-ticks. Same for other elements of JSON.

28

Please use double back-ticks for language constructs like this.

31

Please highlight std::vector::swap with double back-ticks.

veluca93 updated this revision to Diff 427007.May 4 2022, 7:52 AM
veluca93 marked 7 inline comments as done.

Address first round of review comments.

veluca93 marked an inline comment as done.May 4 2022, 7:53 AM
Eugene.Zelenko added inline comments.May 4 2022, 8:07 AM
clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.cpp
136

const auto *. Same in other similar places.

veluca93 updated this revision to Diff 427013.May 4 2022, 8:08 AM

Missing const from auto *

veluca93 marked an inline comment as done.May 4 2022, 8:09 AM
veluca93 added inline comments.
clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.cpp
136

Somehow I missed a couple of those... should be fixed now.

clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
26–27

The quotes here don't feel like they're needed.

36

Single backquotes around FunctionAllowList

94

Since std::string is just a type alias, shouldn't we be considering basic_string?
What about wstring?

104

string_view is also just a type alias, there is wstring_view, etc.

116–153

It would be easier to scan this list if it were sorted alphabetically. I see basic_string listed here, but not basic_string_view. Is that intentional?

173–174

This isn't user-friendly at all. Why not an array of argument indices starting at zero?

clang-tools-extra/test/clang-tidy/checkers/performance-unused-no-side-effect.cpp
119

Should there be some test cases for user-defined template types with no side effects?

Also some test cases for the smart pointer cases?

veluca93 updated this revision to Diff 429745.May 16 2022, 9:18 AM
veluca93 marked 6 inline comments as done.

Switch from bitmasks to arrays. Add more tests & update doc.

clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
94

Looks like std::string is indeed unnecessary.

173–174

Done. I'm using an empty array to indicate "everything", which is perhaps a bit weird.

clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
173–174

I think [-1] might be a better sentinel. What do you do for functions that take zero arguments?

veluca93 marked an inline comment as done.May 17 2022, 9:38 AM
veluca93 added inline comments.
clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
173–174

It doesn't matter, right? For the purpose of tracking whether variables are used, 0-argument free/static functions are meaningless anyway.

clang-tools-extra/docs/clang-tidy/checks/performance-unused-no-side-effect.rst
173–174

OK, yes, I was thinking member functions not free/static functions.

veluca93 marked 2 inline comments as done.May 30 2022, 9:00 AM

@LegalizeAdulthood friendly ping :)

Luca,

Can you expand the description to give a better intuition as to what this check is finding that the diagnostic does not? The example is good, but it would be helpful if you could phrase the general rule being enforced. Similarly, that would be helpful in the documentation file -- before diving into the formal specification, explain what the intuition. For that matter, why does this merit a separate clang-tidy vs. integeration into the existing code for the diagnostic (or a similar, parallel one)? I'm not advocating one way or another, I'd just like to understand your decision.

Separately, can you provide some data as to how much of a problem this is in practice? The code here is quite complex and I think that sets a high bar (in terms of benefit to users) for committing it.

Thanks!

clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.h
53–56

Please add comments explaining the roles of these fields (either individually or collectively)

57

can you expand or rephrase? is the idea that these are argument positions that should be ignored for the purposes of this check? If so, what does the name of the field mean?

59–60

Might this be better in the .cpp file?

clang-tools-extra/docs/clang-tidy/checks/list.rst
117

why the change to this line?

asoffer added inline comments.Jun 3 2022, 6:56 AM
clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.cpp
91

Perhaps I'm not understanding precisely what markSideEffectFree means, but this doesn't seem right to me:

int *data = ...;
auto getNextEntry = [&] () -> int& { return *++data; };
int n = (getNextDataEntry() + 17); // We can't mark RHS as side-effect free.
*data = n;
veluca93 marked 5 inline comments as done.Jun 3 2022, 7:38 AM

Luca,

Can you expand the description to give a better intuition as to what this check is finding that the diagnostic does not? The example is good, but it would be helpful if you could phrase the general rule being enforced. Similarly, that would be helpful in the documentation file -- before diving into the formal specification, explain what the intuition. For that matter, why does this merit a separate clang-tidy vs. integeration into the existing code for the diagnostic (or a similar, parallel one)? I'm not advocating one way or another, I'd just like to understand your decision.

I wrote in the doc:

This check is similar to the -Wunused-variable and -Wunused-but-set-variable
compiler warnings; however, it integrates (configurable) knowledge about
library types and functions to be able to extend these checks to more types of
variables that do not produce user-visible side effects but i.e. perform
allocations; not using such variables is almost always a programming error.

The reason for this to be a tidy rather than an extension of the warning is twofold:

  • I'm not quite confident that this check meets (or can meet) the standard for FPR that warnings are held to. It may well be the case, but it's not something I checked extensively.
  • I don't think compiler warnings can be configured to the extent that would be needed for this check to be useful.

Separately, can you provide some data as to how much of a problem this is in practice? The code here is quite complex and I think that sets a high bar (in terms of benefit to users) for committing it.

I have seen this check trigger about 100k times among ~100M analyzed lines of code.

Thanks!

clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.cpp
91

markSideEffectFree is a bad name - I renamed it to markMaybeNotObservable, with the idea being that an expression is observable iff any of its ancestors in the AST is marked as Observable.

clang-tools-extra/clang-tidy/performance/UnusedNoSideEffectCheck.h
57

I renamed the field and expanded the comment.

clang-tools-extra/docs/clang-tidy/checks/list.rst
117

No idea, must have been a merge gone wrong :) Reverted.

veluca93 updated this revision to Diff 434026.Jun 3 2022, 7:38 AM
veluca93 marked 3 inline comments as done.

Respond to comments

LegalizeAdulthood requested changes to this revision.Jun 22 2022, 2:11 PM

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto main:HEAD and:

  • fold your changes into the appropriate subdirs, stripping the module prefix from new files.
  • make the target check-clang-extra to validate your tests
  • make the target docs-clang-tools-html to validate any docs changes
This revision now requires changes to proceed.Jun 22 2022, 2:11 PM