[clang] [clang][dataflow] Change `diagnoseFunction` to take type of diagnostic list instead of diagnostic itself. (PR #66014)
martinboehme wrote: > Updated to enforce llvm::SmallVector, per Martin's recommendation. PTAL. LGTM, but note that the PR title is now out of date. https://github.com/llvm/llvm-project/pull/66014 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Change `diagnoseFunction` to take type of diagnostic list instead of diagnostic itself. (PR #66014)
https://github.com/martinboehme edited https://github.com/llvm/llvm-project/pull/66014 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Change `diagnoseFunction` to take type of diagnostic list instead of diagnostic itself. (PR #66014)
https://github.com/Xazax-hun approved this pull request. https://github.com/llvm/llvm-project/pull/66014 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Change `diagnoseFunction` to take type of diagnostic list instead of diagnostic itself. (PR #66014)
@@ -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( martinboehme wrote: ```suggestion template llvm::Expected> diagnoseFunction( ``` See also PR-level comment with rationale. https://github.com/llvm/llvm-project/pull/66014 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Change `diagnoseFunction` to take type of diagnostic list instead of diagnostic itself. (PR #66014)
https://github.com/martinboehme edited https://github.com/llvm/llvm-project/pull/66014 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Change `diagnoseFunction` to take type of diagnostic list instead of diagnostic itself. (PR #66014)
https://github.com/martinboehme requested changes to this pull request. https://github.com/llvm/llvm-project/pull/66014 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Change `diagnoseFunction` to take type of diagnostic list instead of diagnostic itself. (PR #66014)
https://github.com/martinboehme review_requested https://github.com/llvm/llvm-project/pull/66014 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Change `diagnoseFunction` to take type of diagnostic list instead of diagnostic itself. (PR #66014)
martinboehme wrote: I'm not convinced of the value of this change. Wouldn't it be simpler to keep the template arguments of `diagnoseFunction()` unchanged and instead change the return type to be `llvm::Expected>`? In other words, always return an `llvm::SmallVector` instead of a `std::vector`? I think `llvm::SmallVector` makes sense for both callsites that currently exist (and ones that we might add in the future). This patch currently lets `UncheckedOptionalAccessCheck::check()` continue to use `std::vector`, but I think `llvm::SmallVector` makes sense there as well, as in the common case, we expect only a small number of diagnostics or none at all. This seems like it would be true for any function that wants to call `diagnoseFunction()` in the future as well. https://github.com/llvm/llvm-project/pull/66014 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Change `diagnoseFunction` to take type of diagnostic list instead of diagnostic itself. (PR #66014)
https://github.com/Xazax-hun approved this pull request. LGTM! https://github.com/llvm/llvm-project/pull/66014 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Change `diagnoseFunction` to take type of diagnostic list instead of diagnostic itself. (PR #66014)
https://github.com/ymand review_requested https://github.com/llvm/llvm-project/pull/66014 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Change `diagnoseFunction` to take type of diagnostic list instead of diagnostic itself. (PR #66014)
https://github.com/llvmbot labeled https://github.com/llvm/llvm-project/pull/66014 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Change `diagnoseFunction` to take type of diagnostic list instead of diagnostic itself. (PR #66014)
https://github.com/ymand review_requested https://github.com/llvm/llvm-project/pull/66014 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Change `diagnoseFunction` to take type of diagnostic list instead of diagnostic itself. (PR #66014)
https://github.com/ymand created https://github.com/llvm/llvm-project/pull/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. >From 6eda5a1fc6200027d6ae99dc5eff69aa88962c81 Mon Sep 17 00:00:00 2001 From: Yitzhak Mandelbaum Date: Mon, 11 Sep 2023 21:34:00 + Subject: [PATCH] [clang][dataflow] Change `diagnoseFunction` to take type of diagnostic list instead of diagnostic itself. 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. --- .../clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp | 4 ++-- .../clang/Analysis/FlowSensitive/DataflowAnalysis.h | 8 .../FlowSensitive/TypeErasedDataflowAnalysisTest.cpp | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) 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 : *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 , ASTContext , -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 , 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`. ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits