[PATCH] D131438: [clang][dataflow] Analyze constructor bodies

[Sam Estep via Phabricator via cfe-commits]

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