Thread safety analysis: provide printSCFG definition. #80277
Conversation
I'm calling this function when investigating the issue (llvm#78131), and I'm surprised to see the definition is commented out. I think it makes sense to provide the definition even though the implementation is not stable.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-analysis Author: Haojian Wu (hokein) ChangesI called this function when investigating the issue (#78131), and I was surprised to see the definition is commented out. I think it makes sense to provide the definition even though the implementation is not stable. Full diff: https://github.com/llvm/llvm-project/pull/80277.diff 2 Files Affected:
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
index 13e37ac2b56b6..4edd3374dd61b 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -528,6 +528,7 @@ class SExprBuilder {
};
// Dump an SCFG to llvm::errs().
+// The implementation is not stable, and used for debugging only.
void printSCFG(CFGWalker &Walker);
} // namespace threadSafety
diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp
index 2fe0f85897c3b..fc5b7d3b6f197 100644
--- a/clang/lib/Analysis/ThreadSafetyCommon.cpp
+++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp
@@ -995,7 +995,6 @@ void SExprBuilder::exitCFG(const CFGBlock *Last) {
IncompleteArgs.clear();
}
-/*
namespace {
class TILPrinter :
@@ -1016,4 +1015,3 @@ void printSCFG(CFGWalker &Walker) {
} // namespace threadSafety
} // namespace clang
-*/
|
|
It might have been commented out so that it doesn't take up space in the compiled binary. I'd like seeing it compiled, just to make sure it doesn't break. But I'd also like if it doesn't appear in the final binary. Perhaps we can change visibility so that |
If we don't want it to be in the final binary, how about guarding this part of code ( |
|
This sounds like a good idea! |
Done. |
I called this function when investigating the issue (#78131), and I was surprised to see the definition is commented out.
I think it makes sense to provide the definition even though the implementation is not stable.