[PATCH] D130600: [clang][dataflow] Handle return statements

[Eric Li via Phabricator via cfe-commits]

li.zhe.hua added inline comments.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h:114
+  ///
+  ///  `return` must not be assigned a storage location.
+  void setReturnStorageLocation(StorageLocation &Loc) {
----------------
samestep wrote:
> li.zhe.hua wrote:
> > Fix this as well? A reader shouldn't need to root around in private implementation details to understand the requirements for calling a function.
> Could you clarify what you mean? I was simply copying the signature and docstring from `setThisPointeeStorageLocation`.
You've marked `return` in backticks. There is no parameter named `return` and it is unclear what `return` refers to. My best guess is that this is a typo of `ReturnLoc`, which is a private data member. So this is a public interface with a requirement that a private data member has some property. This should instead reframe the requirement as properties from the external reader's perspective.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:3908
+      Var = true;
+      return;
+    }
----------------
samestep wrote:
> li.zhe.hua wrote:
> > Why is this change to the test necessary?
> This is mentioned in the patch description (updated earlier today); it's not necessary, but I added it to get a bit of extra coverage for some cases in the `VisitReturnStmt` method this patch adds.
Please add the coverage as a separate test. Separate behaviors should be tested as separate tests. go/unit-testing-practices#behavior-testing


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130600