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

[Sam Estep via Phabricator via cfe-commits]

samestep 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) {
----------------
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`.


================
Comment at: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp:221
+
   // FIXME: Currently this only works if the callee is never a method and the
   // same callee is never analyzed from multiple separate callsites. To
----------------
li.zhe.hua wrote:
> I'm a little unclear on what "this" is. Is it this entire function, or just the call to `getDirectCallee()`? Can this comment be moved somewhere more appropriate, and specifically, so it is touching the code that is most relevant to it? It is currently floating in the middle of the function, and it's unclear to me why new code is being added above it vs. below it.
Yes, this comment is not meant to be present in this patch; I am trying to update the parent patch to remove this comment, but am currently unable to do export it to Phabricator for technical reasons. I should be able to do that tomorrow.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:3908
+      Var = true;
+      return;
+    }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130600