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