Move the TBR analysis in DiffRequest and provide an interface to query it. #971
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -224,7 +224,7 @@ class TBRAnalyzer : public clang::RecursiveASTVisitor<TBRAnalyzer> { | |||
| enum Mode { kMarkingMode = 1, kNonLinearMode = 2 }; | |||
| /// Tells if the variable at a given location is required to store. Basically, | |||
| /// is the result of analysis. | |||
| std::set<clang::SourceLocation> m_TBRLocs; | |||
| std::set<clang::SourceLocation>& m_TBRLocs; | |||
There was a problem hiding this comment.
warning: member 'm_TBRLocs' of type 'std::setclang::SourceLocation &' is a reference [cppcoreguidelines-avoid-const-or-ref-data-members]
std::set<clang::SourceLocation>& m_TBRLocs;
^There was a problem hiding this comment.
This is bogus. cppcoreguidelines-avoid-const-or-ref-data-members says "his check warns when structs or classes that are copyable or movable, and have const-qualified or reference (lvalue or rvalue) data members.". The TBRAnalyzer has deleted copy and move operations but the complain is still on. I suspect there is a bug in clang-tidy...
|
@PetroZarytskyi, this change triggers Do you have a clue why that happens? |
| assert(EnableTBRAnalysis && "TBR not enabled!"); | ||
|
|
||
| if (!isa<DeclRefExpr>(E) || !isa<ArraySubscriptExpr>(E) || | ||
| !isa<MemberExpr>(E)) |
There was a problem hiding this comment.
I think the condition should be !isa<DeclRefExpr>(E) && !isa<ArraySubscriptExpr>(E) && !isa<MemberExpr>(E).
DeclRefExpr, ArraySubscriptExpr, and MemberExpr are different expression types so at least one of the three conditions !isa<... will always be true. Because of this, the function always returns true here. This is why we get the Unexpectedly Passed check (TBR analysis is essentially turned off).
|
Other than my last comment, the PR looks good to me. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #971 +/- ##
==========================================
- Coverage 93.98% 93.90% -0.08%
==========================================
Files 55 55
Lines 8012 8013 +1
==========================================
- Hits 7530 7525 -5
- Misses 482 488 +6
... and 2 files with indirect coverage changes
|
…y it. The idea of this change is to separate the analysis steps from the building step. That would help us detect unsupported cases and provide useful diagnostics without crashing. The centralization will help with enabling the future activity analysis in other modes, too. Partially addresses #721.
|
The failure on the ubuntu llvm-18 bot is due to some apt-get problem. I do not understand why codecov is not happy with the test coverage and at the same time it complains about reduced coverage. Maybe if we removed code the coverage computation goes wrong... |
The idea of this change is to separate the analysis steps from the building step. That would help us detect unsupported cases and provide useful diagnostics without crashing. The centralization will help with enabling the future activity analysis in other modes, too.
Partially addresses #721.