aaron.ballman added inline comments.
================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:188
+ diag(OwnerAssignment->getLocStart(),
+ "assigning neither an owner nor a recognized resource")
+ << SourceRange(OwnerAssignment->getLocStart(),
----------------
JonasToth wrote:
> aaron.ballman wrote:
> > Same comments here as above about "owner" and "recognized resource". I
> > think you want to talk about `gsl::owner<>` where you use "owner" and drop
> > "recognized resource" (because such a resource returns a
> > `gsl::owner<>`-flagged type, if I understand properly).
> ideally every created resource would be flagged as `gsl::owner<>`, but for
> example `fopen`, `malloc` and friends can't, but would benefit the most from
> such a check (+ flow analysis if the owner gets consumed on every path)
The use case certainly makes sense, but that doesn't make the diagnostic any
more clear to the user. Perhaps: "expected assignment source to be of type
'gsl::owner<>'; got %0"? It's not perfect because it doesn't mention
"recognized resource", but that can be improved in the follow-up patch that
defines those resources.
The wording should be similar for the below cases as well.
================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:167
+ "expected argument of type 'gsl::owner<>'; got %0")
+ << ExpectedOwner->getType().getAsString()
+ << ExpectedOwner->getSourceRange();
----------------
You can pass the `QualType` directly instead of calling `getAsString()` on it.
================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:184
+ // Assignments to owners.
+ if (OwnerAssignment != nullptr) {
+ diag(OwnerAssignment->getLocStart(),
----------------
No need to check against `nullptr` explicitly (here and elsewhere).
================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:191
+ // Initialization of owners.
+ else if (OwnerInitialization != nullptr) {
+ diag(OwnerInitialization->getLocStart(),
----------------
No else after a return. Same applies elsewhere.
================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:208
+
+bool OwningMemoryCheck::handleBadAssignmentAndInit(const BoundNodes &Nodes) {
+ // Problematic assignment and initializations, since the assigned value is a
----------------
It's hard to understand how this differs from `handleAssignmentAndInit()`;
might need a more descriptive name.
================
Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:230
+ << BadOwnerInitialization->getSourceRange();
+ // FIXME FixitHint to rewrite the type if possible
+
----------------
Are you intending to address this in this patch?
Also, it should probably be "FIXME: " (with the colon).
https://reviews.llvm.org/D36354
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
