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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
