[PATCH] D65120: More warnings regarding gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/lib/Sema/SemaInit.cpp:6563
 
+static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) {
+  if (auto *Conv = dyn_cast_or_null<CXXConversionDecl>(Callee))
----------------
gribozavr wrote:
> This looks like an ad-hoc rule. Is there a way to express it with attributes instead?
It is indeed an ad-hoc rule. The point is that we do know that `std` methods behave this way and function attributes will be able to express the same in the future. But we are still experimenting with what is the best spelling, representation etc for those function attributes. So given the value of these checks I do not want to wait until function annotations are upstreamed since we can achieve the same with relatively little code. I run these checks on ~300 open source projects and not a single false positive was found in the latest run. 


================
Comment at: clang/lib/Sema/SemaInit.cpp:6572
+    return false;
+  return llvm::StringSwitch<bool>(Callee->getName())
+      .Cases("begin", "rbegin", "cbegin", "crbegin", true)
----------------
gribozavr wrote:
> Should we also check that `Callee->getParent` is an owner?
> 
> Otherwise the check would complain about `begin()` on, for example, a `std::string_view`.
This is intentional. We only warn if the initialization chain can be tracked back to a temporary (or local in some cases) owner. 
So we warn for the following code:
```
const char *trackThroughMultiplePointer() {
  return std::basic_string_view<char>(std::basic_string<char>()).begin(); // expected-warning {{returning address of local temporary object}}
}
```


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

https://reviews.llvm.org/D65120