[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) {
----------------
sgatev wrote:
> li.zhe.hua wrote:
> > 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.
> That was my guess initially too, but my next best guess is that `return` in backticks stands for the keyword/AST node. In any case, let's make it less ambiguous and let's not add requirements based on implementation details. How about: `The return value must not be assigned a storage location.`?
Ah sorry, I understand now; you simply meant that I should make the same change here that @gribozavr2 suggested I make to the other docstrings? I'll go ahead and do that (once I am able to re-export this patch again).


================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:338-339
+    if (Loc == nullptr) {
+      // The outermost context does not set a storage location for `return`, so
+      // in that case we just ignore `return` statements.
+      return;
----------------
sgatev wrote:
> samestep wrote:
> > sgatev wrote:
> > > Let's make this a FIXME to set a storage location for the outermost context too.
> > @sgatev I could add a `FIXME` for that, or I could just do it in this same patch; do you have a preference between those two options?
> Same patch works!
Cool, I have an update to this patch which does that, so I'll export it here as soon as I am able to do so.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:3908
+      Var = true;
+      return;
+    }
----------------
li.zhe.hua wrote:
> 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
Makes sense, I'll do that. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130600