[clang][dataflow] Change diagnoseFunction to use llvm::SmallVector instead of std::vector.
#66014
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis ChangesThe template is agnostic as to the type used by the list, as long as it is -- 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
index 183beb6bfb87d80..5811e2a4cd02266 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
@@ -56,8 +56,8 @@ void UncheckedOptionalAccessCheck::check(
// `diagnoseFunction` with config options.
if (llvm::Expected> Locs =
dataflow::diagnoseFunction(*FuncDecl, *Result.Context,
- Diagnoser))
+ std::vector>(
+ *FuncDecl, *Result.Context, Diagnoser))
for (const SourceLocation &Loc : *Locs)
diag(Loc, "unchecked access to optional value");
else
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
index abd34f40922121e..fd2d7fce09073bf 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
@@ -245,10 +245,10 @@ runDataflowAnalysis(
/// iterations.
/// - This limit is still low enough to keep runtimes acceptable (on typical
/// machines) in cases where we hit the limit.
-template
-llvm::Expected> diagnoseFunction(
+template
+llvm::Expected diagnoseFunction(
const FunctionDecl &FuncDecl, ASTContext &ASTCtx,
- llvm::function_ref(
+ llvm::function_ref &)>
Diagnoser,
@@ -263,7 +263,7 @@ llvm::Expected> diagnoseFunction(
DataflowAnalysisContext AnalysisContext(std::move(OwnedSolver));
Environment Env(AnalysisContext, FuncDecl);
AnalysisT Analysis(ASTCtx);
- std::vector Diagnostics;
+ DiagnosticList Diagnostics;
if (llvm::Error Err =
runTypeErasedDataflowAnalysis(
*Context, Analysis, Env,
diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
index af34a2afd24685a..3a8253508016caa 100644
--- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -98,12 +98,12 @@ TEST(DataflowAnalysisTest, DiagnoseFunctionDiagnoserCalledOnEachElement) {
cast(findValueDecl(AST->getASTContext(), "target"));
auto Diagnoser = [](const CFGElement &Elt, ASTContext &,
const TransferStateForDiagnostics &) {
- std::vector Diagnostics(1);
+ llvm::SmallVector Diagnostics(1);
llvm::raw_string_ostream OS(Diagnostics.front());
Elt.dumpToStream(OS);
return Diagnostics;
};
- auto Result = diagnoseFunction(
+ auto Result = diagnoseFunction>(
*Func, AST->getASTContext(), Diagnoser);
// `diagnoseFunction` provides no guarantees about the order in which elements
// are visited, so we use `UnorderedElementsAre`.
|
|
@llvm/pr-subscribers-clang ChangesThe template is agnostic as to the type used by the list, as long as it is -- 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
index 183beb6bfb87d80..5811e2a4cd02266 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
@@ -56,8 +56,8 @@ void UncheckedOptionalAccessCheck::check(
// `diagnoseFunction` with config options.
if (llvm::Expected> Locs =
dataflow::diagnoseFunction(*FuncDecl, *Result.Context,
- Diagnoser))
+ std::vector>(
+ *FuncDecl, *Result.Context, Diagnoser))
for (const SourceLocation &Loc : *Locs)
diag(Loc, "unchecked access to optional value");
else
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
index abd34f40922121e..fd2d7fce09073bf 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
@@ -245,10 +245,10 @@ runDataflowAnalysis(
/// iterations.
/// - This limit is still low enough to keep runtimes acceptable (on typical
/// machines) in cases where we hit the limit.
-template
-llvm::Expected> diagnoseFunction(
+template
+llvm::Expected diagnoseFunction(
const FunctionDecl &FuncDecl, ASTContext &ASTCtx,
- llvm::function_ref(
+ llvm::function_ref &)>
Diagnoser,
@@ -263,7 +263,7 @@ llvm::Expected> diagnoseFunction(
DataflowAnalysisContext AnalysisContext(std::move(OwnedSolver));
Environment Env(AnalysisContext, FuncDecl);
AnalysisT Analysis(ASTCtx);
- std::vector Diagnostics;
+ DiagnosticList Diagnostics;
if (llvm::Error Err =
runTypeErasedDataflowAnalysis(
*Context, Analysis, Env,
diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
index af34a2afd24685a..3a8253508016caa 100644
--- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -98,12 +98,12 @@ TEST(DataflowAnalysisTest, DiagnoseFunctionDiagnoserCalledOnEachElement) {
cast(findValueDecl(AST->getASTContext(), "target"));
auto Diagnoser = [](const CFGElement &Elt, ASTContext &,
const TransferStateForDiagnostics &) {
- std::vector Diagnostics(1);
+ llvm::SmallVector Diagnostics(1);
llvm::raw_string_ostream OS(Diagnostics.front());
Elt.dumpToStream(OS);
return Diagnostics;
};
- auto Result = diagnoseFunction(
+ auto Result = diagnoseFunction>(
*Func, AST->getASTContext(), Diagnoser);
// `diagnoseFunction` provides no guarantees about the order in which elements
// are visited, so we use `UnorderedElementsAre`.
|
|
@llvm/pr-subscribers-clang-tidy ChangesThe template is agnostic as to the type used by the list, as long as it is -- 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
index 183beb6bfb87d80..5811e2a4cd02266 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
@@ -56,8 +56,8 @@ void UncheckedOptionalAccessCheck::check(
// `diagnoseFunction` with config options.
if (llvm::Expected> Locs =
dataflow::diagnoseFunction(*FuncDecl, *Result.Context,
- Diagnoser))
+ std::vector>(
+ *FuncDecl, *Result.Context, Diagnoser))
for (const SourceLocation &Loc : *Locs)
diag(Loc, "unchecked access to optional value");
else
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
index abd34f40922121e..fd2d7fce09073bf 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
@@ -245,10 +245,10 @@ runDataflowAnalysis(
/// iterations.
/// - This limit is still low enough to keep runtimes acceptable (on typical
/// machines) in cases where we hit the limit.
-template
-llvm::Expected> diagnoseFunction(
+template
+llvm::Expected diagnoseFunction(
const FunctionDecl &FuncDecl, ASTContext &ASTCtx,
- llvm::function_ref(
+ llvm::function_ref &)>
Diagnoser,
@@ -263,7 +263,7 @@ llvm::Expected> diagnoseFunction(
DataflowAnalysisContext AnalysisContext(std::move(OwnedSolver));
Environment Env(AnalysisContext, FuncDecl);
AnalysisT Analysis(ASTCtx);
- std::vector Diagnostics;
+ DiagnosticList Diagnostics;
if (llvm::Error Err =
runTypeErasedDataflowAnalysis(
*Context, Analysis, Env,
diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
index af34a2afd24685a..3a8253508016caa 100644
--- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -98,12 +98,12 @@ TEST(DataflowAnalysisTest, DiagnoseFunctionDiagnoserCalledOnEachElement) {
cast(findValueDecl(AST->getASTContext(), "target"));
auto Diagnoser = [](const CFGElement &Elt, ASTContext &,
const TransferStateForDiagnostics &) {
- std::vector Diagnostics(1);
+ llvm::SmallVector Diagnostics(1);
llvm::raw_string_ostream OS(Diagnostics.front());
Elt.dumpToStream(OS);
return Diagnostics;
};
- auto Result = diagnoseFunction(
+ auto Result = diagnoseFunction>(
*Func, AST->getASTContext(), Diagnoser);
// `diagnoseFunction` provides no guarantees about the order in which elements
// are visited, so we use `UnorderedElementsAre`.
|
|
I'm not convinced of the value of this change. Wouldn't it be simpler to keep the template arguments of I think |
martinboehme
requested changes
Sep 12, 2023
FWIW, I also plan to followup with exactly that change to |
|
Updated to enforce |
Xazax-hun
approved these changes
Sep 12, 2023
LGTM, but note that the PR title is now out of date. |
ymand
changed the title
diagnoseFunction to take type of diagnostic list instead of diagnostic itself.diagnoseFunction to use llvm::SmallVector instead of std::vector.
martinboehme
approved these changes
Sep 13, 2023
ymand
force-pushed
the
generalize-template
branch
from
64c65d8
to
8b73f91
Compare
…` instead of `std::vector`. The template is agnostic as to the type used by the list, as long as it is compatible with `llvm::move` and `std::back_inserter`. `llvm::SmallVector` seems generally more appropriate than `std::vector` in this context, so we set that as the expected list type.
ymand
force-pushed
the
generalize-template
branch
from
8b73f91
to
a82c9f3
Compare
ZijunZhaoCCK
pushed a commit
to ZijunZhaoCCK/llvm-project
that referenced
this pull request
Sep 19, 2023
…` instead of `std::vector`. (llvm#66014) The template is agnostic as to the type used by the list, as long as it is compatible with `llvm::move` and `std::back_inserter`. In practice, we've encountered analyses which use different types (`llvm::SmallVector` vs `std::vector`), so it seems preferable to leave this open to the caller.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The template is agnostic as to the type used by the list, as long as it is
compatible with
llvm::moveandstd::back_inserter. In practice, we'veencountered analyses which use different types (
llvm::SmallVectorvsstd::vector), so it seems preferable to leave this open to the caller.