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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev 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:
> 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.`?


================
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;
----------------
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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130600