[PATCH] D137432: [clang][dataflow] Change transfer and diagnoser functions to receive a CFGElement

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Thanks for this patch!  Unfortunately, I'm having trouble following the change.  Can you expand the description to give a more detailed explantion of the motivation and the key point that are changing?  Unfortunately, I don't understand the motivation for the new API and am even having trouble finding where the changes were made that correspond to the new API used by clients.  Maybe that would be good to include in the description? That is, call out the key old API that's being upgraded and write the explanation with respect to the old and the new?

Thanks!



================
Comment at: clang/include/clang/Analysis/FlowSensitive/CFGMatchSwitch.h:33
 
-template <typename State, typename Result = void>
-using CFGMatchSwitch =
-    std::function<Result(const CFGElement &, ASTContext &, State &)>;
+template <typename State, typename Result = void> class CFGMatchSwitch {
+public:
----------------
nit: just wanted to double check on the formatting: i'm used to seeing `class` put on a new line.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/CFGMatchSwitch.h:35
+public:
+  CFGMatchSwitch(std::unique_ptr<const CFGElement *> CurrentElement,
+                 ASTMatchSwitch<Stmt, State, Result> StmtMS,
----------------
xazax.hun wrote:
> Why do we need to keep this pointer on the heap?
Can you please explain the need for the unique pointer?


================
Comment at: clang/include/clang/Analysis/FlowSensitive/CFGMatchSwitch.h:42
+  Result operator()(const CFGElement &Element, ASTContext &Context, State &S) {
+    *CurrentElement = ∈
+    switch (Element.getKind()) {
----------------
Where is this field used?


================
Comment at: clang/include/clang/Analysis/FlowSensitive/CFGMatchSwitch.h:150-151
   ASTMatchSwitchBuilder<CXXCtorInitializer, State, Result> InitBuilder;
+  std::unique_ptr<const CFGElement *> CurrentElement =
+      std::make_unique<const CFGElement *>(nullptr);
 };
----------------
Please add a comment explaining the design pattern. I'm not quite clear on why we're taking this approach, so it would probably be generally useful to explain to future readers.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h:52
 template <typename T>
 using MatchSwitchMatcher = ast_matchers::internal::Matcher<T>;
 
----------------
should this change too?


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:280
 
-void initializeOptionalReference(const Expr *OptionalExpr,
+void initializeOptionalReference(const CFGElement &Element,
+                                 const Expr *OptionalExpr,
----------------
ymandel wrote:
> This seems a surprising API choice. Once the callee is getting the element as statement at a specific type, why does it need the element?
Here and below: if the callee does not use the paramter, drop its name.


================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:280-281
 
-void initializeOptionalReference(const Expr *OptionalExpr,
+void initializeOptionalReference(const CFGElement &Element,
+                                 const Expr *OptionalExpr,
                                  const MatchFinder::MatchResult &,
----------------
This seems a surprising API choice. Once the callee is getting the element as statement at a specific type, why does it need the element?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137432