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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits