[PATCH] D132763: [clang][dataflow] Use `StringMap` for storing analysis states at annotated points instead of `vector<pair<string, StateT>>`.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 accepted this revision.
gribozavr2 added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp:124
+      }
+      Result[Stmt] = Annotation;
+
----------------



================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:262
     std::function<void(
-        llvm::ArrayRef<std::pair<
-            std::string, DataflowAnalysisState<typename AnalysisT::Lattice>>>,
+        llvm::StringMap<DataflowAnalysisState<typename AnalysisT::Lattice>> &,
         AnalysisOutputs &)>
----------------



================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:300
         llvm::any_cast<typename AnalysisT::Lattice>(&State.Lattice.Value);
-    AnnotationStates.emplace_back(It->second, StateT{*Lattice, State.Env});
+    AnnotationStates.insert({It->second, StateT{*Lattice, State.Env}});
   };
----------------
Could you add an assert that insert reported that it indeed inserted an element (didn't overwrite)?


================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:356-357
+            AnnotationStatesAsVector;
+        for (auto It = AnnotationStates.begin(); It != AnnotationStates.end();
+             It++) {
+          AnnotationStatesAsVector.push_back(
----------------
Would a range-based for loop work?

`for (const auto &P : AnnotationStates)`


================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:358-361
+          AnnotationStatesAsVector.push_back(
+              std::pair<std::string,
+                        DataflowAnalysisState<typename AnalysisT::Lattice>>(
+                  It->first(), std::move(It->second)));
----------------
Would make_pair work? It allows to omit explicit template arguments.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:363-365
+        std::sort(AnnotationStatesAsVector.begin(),
+                  AnnotationStatesAsVector.end(),
+                  [](auto a, auto b) { return a.first < b.first; });
----------------
Please use llvm::sort. You also don't need to specify the comparator, the default comparator for strings is exactly the same.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132763/new/

https://reviews.llvm.org/D132763