Page MenuHomePhabricator

More warnings regarding gsl::Pointer and gsl::Owner attributes

Authored by xazax.hun on Jul 22 2019, 3:31 PM.



This patch extends the warnings for additional cases: when we call some well-known methods on a temporary object.

This is using a hard coded list of those well-known functions. Later revisions might replace string matching with checking function annotations from the lifetime papers. There are two reasons to hard code the list this way for now:

  1. The spelling and semantics of function annotations might change in the near future after incorporating some implementation related experience so we are not rushing adding those annotations to clang just yet (as opposed to class annotations which are considered quite stable now.)
  2. Checking for those well known functions is a huge usability boost for the check.

Diff Detail


Event Timeline

xazax.hun created this revision.Jul 22 2019, 3:31 PM
xazax.hun marked an inline comment as done.
xazax.hun added inline comments.
2 ↗(On Diff #211214)

I had to disable the compiler warnings for this test file as some of the warnings overlapped with the warnings from the clang static analyzer (which is not bad IMO!). I think this is cleaner than adding expected-warining lines that are unrelated to the analyzer.

xazax.hun marked an inline comment as done.Jul 22 2019, 5:20 PM
xazax.hun added inline comments.
6564 ↗(On Diff #211214)

The second param is unused, will fix in the next revision after the first round of reviews.

  • Remove unused parameter.
gribozavr added inline comments.Jul 29 2019, 6:42 AM
6563 ↗(On Diff #211581)

This looks like an ad-hoc rule. Is there a way to express it with attributes instead?

6572 ↗(On Diff #211581)

Should we also check that Callee->getParent is an owner?

Otherwise the check would complain about begin() on, for example, a std::string_view.

170 ↗(On Diff #211581)

Please add a newline.

xazax.hun marked 3 inline comments as done.
xazax.hun edited the summary of this revision. (Show Details)
  • Add new line to end of file.
  • Rebase, calls no longer need special handling.
6563 ↗(On Diff #211581)

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.

6572 ↗(On Diff #211581)

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}}
gribozavr added inline comments.Aug 6 2019, 4:54 AM
6572 ↗(On Diff #211581)

Makes sense, but then we should still check that Callee->getParent is an owner or a pointer.

xazax.hun updated this revision to Diff 214228.Aug 8 2019, 2:04 PM
  • Fix review comments.
xazax.hun marked 4 inline comments as done.Aug 8 2019, 2:04 PM
xazax.hun added inline comments.
6572 ↗(On Diff #211581)

Good point, done.

gribozavr accepted this revision.Aug 9 2019, 1:16 AM
This revision is now accepted and ready to land.Aug 9 2019, 1:16 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2019, 8:15 AM