[PATCH] D128446: [clang][dataflow] Use annotations for optional diagnostic tests
ymandel added a comment. This looks quite nice. I really like how you solved the problem of diagnostics representation/checking. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128446/new/ https://reviews.llvm.org/D128446 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D128446: [clang][dataflow] Use annotations for optional diagnostic tests
gribozavr2 accepted this revision.
gribozavr2 added inline comments.
This revision is now accepted and ready to land.
Comment at:
clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp:1258
+
+ llvm::DenseSet AnnotationLines;
+ for (const auto : Annotations) {
Comment at:
clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp:1264-1267
+// statement spans multiple lines then the test will fail.
Going
+// forward, we should change this to instead just get the
single
+// line number from the annotation itself, rather than looking
+// at the statement it's attached to.
But maybe there's an easy way to do that -- change checkDataflowDiagnosis pass
down the llvm::Annotations objects to this callback? You could even make that
change in the earlier patch in the stack.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128446/new/
https://reviews.llvm.org/D128446
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D128446: [clang][dataflow] Use annotations for optional diagnostic tests
samestep updated this revision to Diff 439415.
samestep added a comment.
- Merge branch 'diagnose-api' into diagnose-test-annotations
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128446/new/
https://reviews.llvm.org/D128446
Files:
clang/unittests/Analysis/FlowSensitive/TestingSupport.h
clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -11,7 +11,6 @@
#include "TestingSupport.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchers.h"
-#include "clang/Analysis/FlowSensitive/SourceLocationsLattice.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/ArrayRef.h"
@@ -28,6 +27,8 @@
using namespace dataflow;
using namespace test;
+using ::testing::ContainerEq;
+
// FIXME: Move header definitions in separate file(s).
static constexpr char CSDtdDefHeader[] = R"(
#ifndef CSTDDEF_H
@@ -1179,12 +1180,6 @@
} // namespace base
)";
-/// Converts `L` to string.
-static std::string ConvertToString(const llvm::DenseSet ,
- const ASTContext ) {
- return L.empty() ? "safe" : "unsafe: " + DebugString(L, Ctx);
-}
-
/// Replaces all occurrences of `Pattern` in `S` with `Replacement`.
static void ReplaceAllOccurrences(std::string , const std::string ,
const std::string ) {
@@ -1205,18 +1200,14 @@
class UncheckedOptionalAccessTest
: public ::testing::TestWithParam {
protected:
- template
- void ExpectLatticeChecksFor(std::string SourceCode,
- LatticeChecksMatcher MatchesLatticeChecks) {
-ExpectLatticeChecksFor(SourceCode, ast_matchers::hasName("target"),
- MatchesLatticeChecks);
+ void ExpectDiagnosticsFor(std::string SourceCode) {
+ExpectDiagnosticsFor(SourceCode, ast_matchers::hasName("target"));
}
private:
- template
- void ExpectLatticeChecksFor(std::string SourceCode,
- FuncDeclMatcher FuncMatcher,
- LatticeChecksMatcher MatchesLatticeChecks) {
+ template
+ void ExpectDiagnosticsFor(std::string SourceCode,
+FuncDeclMatcher FuncMatcher) {
ReplaceAllOccurrences(SourceCode, "$ns", GetParam().NamespaceName);
ReplaceAllOccurrences(SourceCode, "$optional", GetParam().TypeName);
@@ -1260,10 +1251,30 @@
return Diagnosis.diagnose(Stmt, Env);
};
},
-[](llvm::DenseSet Locs,
-ASTContext ) {
- std::string StringifiedDiags = ConvertToString(Locs, Ctx);
- EXPECT_THAT(StringifiedDiags, MatchesLatticeChecks);
+[](llvm::DenseMap ,
+ llvm::DenseSet , ASTContext ) {
+ auto = Ctx.getSourceManager();
+
+ llvm::DenseSet AnnotationLines;
+ for (const auto : Annotations) {
+auto *Stmt = Pair.getFirst();
+AnnotationLines.insert(
+SrcMgr.getPresumedLineNumber(Stmt->getBeginLoc()));
+// We add both the begin and end locations, so that if the
+// statement spans multiple lines then the test will fail. Going
+// forward, we should change this to instead just get the single
+// line number from the annotation itself, rather than looking
+// at the statement it's attached to.
+AnnotationLines.insert(
+SrcMgr.getPresumedLineNumber(Stmt->getEndLoc()));
+ }
+
+ llvm::DenseSet DiagnosticLines;
+ for (SourceLocation : Locs) {
+DiagnosticLines.insert(SrcMgr.getPresumedLineNumber(Loc));
+ }
+
+ EXPECT_THAT(DiagnosticLines, ContainerEq(AnnotationLines));
},
{"-fsyntax-only", "-std=c++17", "-Wno-undefined-inline"},
FileContents);
@@ -1282,65 +1293,55 @@
});
TEST_P(UncheckedOptionalAccessTest, EmptyFunctionBody) {
- ExpectLatticeChecksFor(R"(
+ ExpectDiagnosticsFor(R"(
void target() {
(void)0;
- /*[[check]]*/
}
- )",
- "safe");
+ )");
}
TEST_P(UncheckedOptionalAccessTest, UnwrapUsingValueNoCheck) {
- ExpectLatticeChecksFor(
+ ExpectDiagnosticsFor(
R"(
#include "unchecked_optional_access_test.h"
void target($ns::$optional opt) {
- opt.value();
- /*[[check]]*/
+ opt.value(); // [[unsafe]]
}
- )",
- "unsafe: input.cc:5:7");
+ )");
- ExpectLatticeChecksFor(
+
[PATCH] D128446: [clang][dataflow] Use annotations for optional diagnostic tests
samestep created this revision.
Herald added subscribers: martong, tschuett, xazax.hun.
Herald added a project: All.
samestep requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D128446
Files:
clang/unittests/Analysis/FlowSensitive/TestingSupport.h
clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
===
--- clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -11,7 +11,6 @@
#include "TestingSupport.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchers.h"
-#include "clang/Analysis/FlowSensitive/SourceLocationsLattice.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/ArrayRef.h"
@@ -28,6 +27,8 @@
using namespace dataflow;
using namespace test;
+using ::testing::ContainerEq;
+
// FIXME: Move header definitions in separate file(s).
static constexpr char CSDtdDefHeader[] = R"(
#ifndef CSTDDEF_H
@@ -1179,12 +1180,6 @@
} // namespace base
)";
-/// Converts `L` to string.
-static std::string ConvertToString(const llvm::DenseSet ,
- const ASTContext ) {
- return L.empty() ? "safe" : "unsafe: " + DebugString(L, Ctx);
-}
-
/// Replaces all occurrences of `Pattern` in `S` with `Replacement`.
static void ReplaceAllOccurrences(std::string , const std::string ,
const std::string ) {
@@ -1205,18 +1200,14 @@
class UncheckedOptionalAccessTest
: public ::testing::TestWithParam {
protected:
- template
- void ExpectLatticeChecksFor(std::string SourceCode,
- LatticeChecksMatcher MatchesLatticeChecks) {
-ExpectLatticeChecksFor(SourceCode, ast_matchers::hasName("target"),
- MatchesLatticeChecks);
+ void ExpectDiagnosticsFor(std::string SourceCode) {
+ExpectDiagnosticsFor(SourceCode, ast_matchers::hasName("target"));
}
private:
- template
- void ExpectLatticeChecksFor(std::string SourceCode,
- FuncDeclMatcher FuncMatcher,
- LatticeChecksMatcher MatchesLatticeChecks) {
+ template
+ void ExpectDiagnosticsFor(std::string SourceCode,
+FuncDeclMatcher FuncMatcher) {
ReplaceAllOccurrences(SourceCode, "$ns", GetParam().NamespaceName);
ReplaceAllOccurrences(SourceCode, "$optional", GetParam().TypeName);
@@ -1260,10 +1251,30 @@
return Diagnosis.diagnose(Stmt, Env);
};
},
-[](llvm::DenseSet Locs,
-ASTContext ) {
- std::string StringifiedDiags = ConvertToString(Locs, Ctx);
- EXPECT_THAT(StringifiedDiags, MatchesLatticeChecks);
+[](llvm::DenseMap ,
+ llvm::DenseSet , ASTContext ) {
+ auto = Ctx.getSourceManager();
+
+ llvm::DenseSet AnnotationLines;
+ for (const auto : Annotations) {
+auto *Stmt = Pair.getFirst();
+AnnotationLines.insert(
+SrcMgr.getPresumedLineNumber(Stmt->getBeginLoc()));
+// We add both the begin and end locations, so that if the
+// statement spans multiple lines then the test will fail. Going
+// forward, we should change this to instead just get the single
+// line number from the annotation itself, rather than looking
+// at the statement it's attached to.
+AnnotationLines.insert(
+SrcMgr.getPresumedLineNumber(Stmt->getEndLoc()));
+ }
+
+ llvm::DenseSet DiagnosticLines;
+ for (SourceLocation : Locs) {
+DiagnosticLines.insert(SrcMgr.getPresumedLineNumber(Loc));
+ }
+
+ EXPECT_THAT(DiagnosticLines, ContainerEq(AnnotationLines));
},
{"-fsyntax-only", "-std=c++17", "-Wno-undefined-inline"},
FileContents);
@@ -1282,65 +1293,55 @@
});
TEST_P(UncheckedOptionalAccessTest, EmptyFunctionBody) {
- ExpectLatticeChecksFor(R"(
+ ExpectDiagnosticsFor(R"(
void target() {
(void)0;
- /*[[check]]*/
}
- )",
- "safe");
+ )");
}
TEST_P(UncheckedOptionalAccessTest, UnwrapUsingValueNoCheck) {
- ExpectLatticeChecksFor(
+ ExpectDiagnosticsFor(
R"(
#include "unchecked_optional_access_test.h"
void target($ns::$optional opt) {
- opt.value();
- /*[[check]]*/
+ opt.value(); // [[unsafe]]
}
- )",
- "unsafe: input.cc:5:7");
+ )");
-
