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

[Eric Li via Phabricator via cfe-commits]

li.zhe.hua added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h:64
   TypeErasedDataflowAnalysis(bool ApplyBuiltinTransfer)
       : Options({ApplyBuiltinTransfer}) {}
 
----------------
Nit: `-Wmissing-field-initializers` is apparently enabled, and starts warning on this.


================
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,
----------------
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?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130306