[PATCH] D131438: [clang][dataflow] Analyze constructor bodies
Sam Estep via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 9 06:39:07 PDT 2022
samestep added inline comments.
================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:138
///
+ /// `Call` must be either a `CallExpr` or a `CXXConstructExpr`.
+ ///
----------------
sgatev wrote:
> How about we define overloads that take these types instead of taking an `Expr` here? This should remove the need for type-checking and guarding against bad input in the implementation. `transferInlineCall` can be a template if necessary.
Hmm I guess we could; is there much of a benefit to doing this templated?
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:678-684
+ if (const auto *NonConstructExpr = dyn_cast<CallExpr>(S)) {
+ // Note that it is important for the storage location of `S` to be set
+ // before `pushCall`, because the latter uses it to set the storage
+ // location for `return`.
+ auto &ReturnLoc = Env.createStorageLocation(*S);
+ Env.setStorageLocation(*S, ReturnLoc);
+ }
----------------
ymandel wrote:
> Why can't this stay in `VisitCallExpr`?
I had it there in an earlier version of this patch; it was causing tests to fail (the `SelfReferential*` ones, if I remember correctly).
================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:4371
+TEST(TransferTest, ContextSensitiveConstructorBody) {
+ std::string Code = R"(
----------------
ymandel wrote:
> What about a default constructor, including when there are field initializers like:
> ```
> class MyClass {
> public:
> MyClass() = default;
>
> bool MyField = true;
> };
> ```
>
> Should we expect to handle that correctly? If so, can you add some tests?
Good idea! I'll add a test for this.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131438/new/
https://reviews.llvm.org/D131438
More information about the cfe-commits
mailing list