[PATCH] D131039: [clang][dataflow] Add cache of `ControlFlowContext`s for function decls.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h:34
+  /// Builds a ControlFlowContext from an AST node. `D` is the function in which
+  /// `S` resides. All arguments must be non-null.
   static llvm::Expected<ControlFlowContext> build(const Decl *D, Stmt *S,
----------------
sgatev wrote:
> ymandel wrote:
> > sgatev wrote:
> > > Can we make them references?
> > Turns out I was wrong - `D` can be null. Would that CFG has comments about what its parameters do... We have a test that excercises this (though I can't say why):
> > 
> > https://github.com/llvm/llvm-project/blob/9a976f36615dbe15e76c12b22f711b2e597a8e51/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp#L70
> > 
> > The other two -- yes -- but then we'd have to update the various callers as well. I'd rather not do that in this patch, but I'll add the new overload and we can follow up with cleanups in another patch.
> > 
> > 
> Weird. I think we should change the test to pass the decl.
I'd like that, but when looking through other uses found a bunch that pass nullptr (and for no apparent reason - they all have the function decl handy).

So, I propose that we mark the new API as such and then in the cleanup we'll make sure to pass a non-null argument.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp:19
 #include "clang/Analysis/FlowSensitive/Value.h"
+#include "clang/Basic/IdentifierTable.h"
 #include "llvm/Support/Debug.h"
----------------
sgatev wrote:
> Is this still needed?
Nope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131039