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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
