[PATCH] D130306: [clang][dataflow] Analyze calls to in-TU functions

[Sam Estep via Phabricator via cfe-commits]

samestep added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h:64
   TypeErasedDataflowAnalysis(bool ApplyBuiltinTransfer)
       : Options({ApplyBuiltinTransfer}) {}
 
----------------
li.zhe.hua wrote:
> Nit: `-Wmissing-field-initializers` is apparently enabled, and starts warning on this.
Ah thanks, will fix.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:42
 void runDataflow(llvm::StringRef Code, Matcher Match,
-                  LangStandard::Kind Std = LangStandard::lang_cxx17,
-                  bool ApplyBuiltinTransfer = true,
-                  llvm::StringRef TargetFun = "target") {
+                 DataflowAnalysisOptions Options,
+                 LangStandard::Kind Std = LangStandard::lang_cxx17,
----------------
li.zhe.hua wrote:
> For the purposes of the test, there's really only 3 states:
> 
>   # No built-in transfer
>   # Built-in transfer, no context-sensitive
>   # Built-in transfer, with context-sensitive
> 
> It may be more readable for tests to have a 3-state enum, that `runDataFlow` will then use to produce the corresponding `DataflowAnalysisOptions`. As is, a snippet like
> 
> ```
>               {/*.ApplyBuiltinTransfer=*/true,
>                /*.BuiltinTransferOptions=*/{/*.ContextSensitive=*/false}});
> ```
> 
> is rough to read. Good enum names with good comments would probably make this much better. WDYT?
I agree that there are only 3 states, but I also think that conceptually this really is a multi-layered thing; either we apply the built-in transfer or not, and then if we do apply the builtin transfer, there's some set of options we pass to it. Thus, it doesn't seem right to just collapse it into a flat enum; I'm not sure though.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:67
+                 llvm::StringRef TargetFun = "target") {
+  runDataflow(Code, Match, {ApplyBuiltinTransfer}, Std, TargetFun);
+}
----------------
li.zhe.hua wrote:
> Nit: `-Wmissing-field-initializers` is apparently enabled, and starts warning on this.
Same here; thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130306