Report all declarations and definitions of unused functions#7
Report all declarations and definitions of unused functions#7eduard-bagdasaryan wants to merge 18 commits intomainfrom
Conversation
This is useful for unused code removal automation.
The reporting now has the last line number of declaration
or definition:
file.cc:8:12: warning: Function 'F' is unused
file.cc:3:3: note: declared here
rousskov
left a comment
There was a problem hiding this comment.
Please add a test case to check the new functionality. Ideally, that test will have multiple definitions of the same function X and multiple declarations of the same function Y, where X and Y are the same (if possible) or different functions (otherwise).
Also report all definitions as well as all declarations.
Added. |
rousskov
left a comment
There was a problem hiding this comment.
Please check updated PR title/description as well.
| // CHECK-DAG: source2.cc:5:{{.*}}definition ends here | ||
|
|
||
| //--- source1.cc | ||
| #include <iostream> |
There was a problem hiding this comment.
I added this extra line so that F() would not start with the first line (avoiding first/last values just in case).
There was a problem hiding this comment.
I do not know what you mean by "avoiding first/last values". I think it would actually be good to test first-line position in one of the source files. In the other file (e.g., this one), let's add a comment instead (if it works):
| #include <iostream> | |
| // no "extern" declaration for F() in this source file |
and remove #include from source2.cc.
main.cpp
Outdated
| if (I.Definitions > 0 && I.Uses == 0) { | ||
| llvm::errs() << I.Filename << ":" << I.Line << ": warning:" | ||
| if (I.declaredOrDefined() && I.Uses == 0) { |
There was a problem hiding this comment.
I suspect that this I.Definitions > 0 condition was meant to skip various functions "external" to the project being scanned (e.g., a system function that the project forward-declares instead of including an appropriate standard header file) and never-defined functions (e.g., forgotten/unused forward declarations of project-specific functions that no longer exist). We did not report such functions as unused prior to this PR. I do not think this PR should start reporting those functions (even though we would like the second category to eventually be included in these "unused" reports).
There was a problem hiding this comment.
Ok, I restored the previous condition (i.e., its equivalent).
Also polished the test case.
main.cpp
Outdated
| if (I.Definitions > 0 && I.Uses == 0) { | ||
| llvm::errs() << I.Filename << ":" << I.Line << ": warning:" | ||
| if (I.declaredOrDefined() && I.Uses == 0) { |
There was a problem hiding this comment.
Ok, I restored the previous condition (i.e., its equivalent).
| // CHECK-DAG: source2.cc:5:{{.*}}definition ends here | ||
|
|
||
| //--- source1.cc | ||
| #include <iostream> |
main.cpp
Outdated
| DefInfo(const size_t uses, const FunctionDecl *F, const SourceManager &SM) | ||
| : Uses(uses), Name(F->getQualifiedNameAsString()) {} |
There was a problem hiding this comment.
In official code, {0,1} and {1,0} construction more-or-less made sense. This PR breaks that symmetry, making constructor-calling code even less readable. I suggest removing uses from the second constructor:
| DefInfo(const size_t uses, const FunctionDecl *F, const SourceManager &SM) | |
| : Uses(uses), Name(F->getQualifiedNameAsString()) {} | |
| // Use this constructor when you find a definition of a never-seen-before function | |
| DefInfo(const FunctionDecl * const F, const SourceManager &SM) | |
| : Uses(0), Name(F->getQualifiedNameAsString()) {} |
main.cpp
Outdated
| DefInfo(const size_t uses, const FunctionDecl *F, const SourceManager &SM) | ||
| : Uses(uses), Name(F->getQualifiedNameAsString()) {} | ||
|
|
||
| bool defined() const { return !Definitions.empty(); } |
There was a problem hiding this comment.
foo.defined() looks a bit odd in source code, as if foo itself is "undefined" in some cases.
| bool defined() const { return !Definitions.empty(); } | |
| bool sawDefinition() const { return !Definitions.empty(); } |
main.cpp
Outdated
| auto &destination = R->doesThisDeclarationHaveABody() ? Definitions : Declarations; | ||
| destination.emplace_back(R, SM); |
There was a problem hiding this comment.
| auto &destination = R->doesThisDeclarationHaveABody() ? Definitions : Declarations; | |
| destination.emplace_back(R, SM); | |
| auto &ds = R->doesThisDeclarationHaveABody() ? Definitions : Declarations; | |
| ds.emplace_back(R, SM); |
eduard-bagdasaryan
left a comment
There was a problem hiding this comment.
Adjusted (b233740).
main.cpp
Outdated
| DefInfo(const size_t uses, const FunctionDecl *F, const SourceManager &SM) | ||
| : Uses(uses), Name(F->getQualifiedNameAsString()) {} |
main.cpp
Outdated
| DefInfo(const size_t uses, const FunctionDecl *F, const SourceManager &SM) | ||
| : Uses(uses), Name(F->getQualifiedNameAsString()) {} | ||
|
|
||
| bool defined() const { return !Definitions.empty(); } |
main.cpp
Outdated
| auto &destination = R->doesThisDeclarationHaveABody() ? Definitions : Declarations; | ||
| destination.emplace_back(R, SM); |
main.cpp
Outdated
| size_t Definitions; | ||
| explicit DefInfo(const size_t uses) : Uses(uses) {} | ||
| // Use this constructor when you find a use of a never-seen-before function | ||
| DefInfo(const FunctionDecl *F, const SourceManager &SM) |
There was a problem hiding this comment.
| DefInfo(const FunctionDecl *F, const SourceManager &SM) | |
| explicit DefInfo(const FunctionDecl * const F) |
main.cpp
Outdated
|
|
||
| it_inserted.first->second.Declarations = getDeclarations(F, SM); | ||
| const auto it_inserted = AllDecls.emplace(std::move(USR), DefInfo(F, SM)); | ||
| const auto &defInfo = it_inserted.first->second; |
There was a problem hiding this comment.
defInfo is unused, right?
| const auto &defInfo = it_inserted.first->second; |
main.cpp
Outdated
| unsigned StartLine; | ||
| unsigned EndLine; |
There was a problem hiding this comment.
Let's avoid "end" because STL end() cannot be dereferenced, but this offset can be.
| unsigned StartLine; | |
| unsigned EndLine; | |
| unsigned FirstLine; | |
| unsigned LastLine; // same as FirstLine for single-line code |
main.cpp
Outdated
| size_t Definitions; | ||
| explicit DefInfo(const size_t uses) : Uses(uses) {} | ||
| // Use this constructor when you find a use of a never-seen-before function | ||
| DefInfo(const FunctionDecl *F, const SourceManager &SM) |
main.cpp
Outdated
|
|
||
| it_inserted.first->second.Declarations = getDeclarations(F, SM); | ||
| const auto it_inserted = AllDecls.emplace(std::move(USR), DefInfo(F, SM)); | ||
| const auto &defInfo = it_inserted.first->second; |
main.cpp
Outdated
| unsigned StartLine; | ||
| unsigned EndLine; |
Duplicated declarations could be stored into DefInfo::Declarations when parsing different TUs. This is the branch-introduced problem: before the PR changes, the Declarations vector was not accumulated but always overwritten by the current TU results.
DefInfo::Name was never initialized when DefInfo was constructed with the first constructor (i.e., when the function's call had been parsed before its declaration/definition).
* Normalize absolute paths to exclude duplicates * Correctly find the beginning of templates * Corectly find the end of function declarations with macro
However, such false possitives still are possible:
//void G();
void F();
Report all declarations and all definitions of an unused function, from
all TUs. Prior to these changes, only the definition in the first TU and
only the declarations from the last TU were reported.
The reporting now also includes the last line number:
These reports help automate unused code removal. They are especially
handy when a large project applies xunused for the first time.