Page MenuHomePhabricator

[clang-tidy] Add check to detect dangling references in value handlers.
ClosedPublic

Authored by sbenza on Mar 2 2016, 8:22 AM.

Details

Summary

Add check misc-dangling-handle to detect dangling references in value
handles like std::experimental::string_view.
It provides a configuration option to specify other handle types that
should also be checked.

Right now it detects:

  • Construction from temporaries.
  • Assignment from temporaries.
  • Return statements from temporaries or locals.
  • Insertion into containers from temporaries.

Diff Detail

Repository
rL LLVM

Event Timeline

sbenza updated this revision to Diff 49629.Mar 2 2016, 8:22 AM
sbenza retitled this revision from to [clang-tidy] Add check to detect dangling references in value handlers..
sbenza updated this object.
sbenza added a reviewer: alexfh.
sbenza added a subscriber: cfe-commits.
sbenza added a subscriber: jbcoe.Mar 2 2016, 8:22 AM
sbenza updated this object.Mar 2 2016, 8:27 AM
jbcoe added a reviewer: jbcoe.Mar 2 2016, 10:49 AM
jbcoe removed a subscriber: jbcoe.
jbcoe added a subscriber: jbcoe.

Does it make D17772 obsolete?

Yes. The other patch has already been abandoned.

jbcoe edited edge metadata.Mar 4 2016, 11:33 AM

I think there are some minor issues with the tests, easily fixed. I wonder if the matchers can catch things inside implementation-defined inline namespaces like std::_v1_::experimental::library_fundamentals_v1::string_view.

clang-tidy/misc/DanglingHandleCheck.cpp
57 ↗(On Diff #49629)

Will this (and similar checkers) handle inline namespaces?

test/clang-tidy/misc-dangling-handle.cpp
73 ↗(On Diff #49629)

What does NOLINT do here?

85 ↗(On Diff #49629)

This (and other) check-messages line looks truncated.

alexfh added inline comments.Mar 7 2016, 5:07 AM
clang-tidy/misc/DanglingHandleCheck.cpp
24 ↗(On Diff #49629)

A very similar code has been added recently to clang-tidy/utils/HeaderFileExtensionsUtils.*. Maybe make that code generic enough and reuse it?

82 ↗(On Diff #49629)

nit: The comment can well fit on a single line without making it less readable. Also, please add a trailing period.

116 ↗(On Diff #49629)

This use of internal classes doesn't look nice. Can you add a hasAnyName overload taking a set or an ArrayRef of names (or maybe a template version that'll take a range of something convertible to StringRef)?

162 ↗(On Diff #49629)

nit: Please add a trailing period (and optionally an ellipsis at the start of the comment and at the end of the previous comment).

175 ↗(On Diff #49629)

handleFromTemporaryValue already binds something to bad, which looks suspicious. Should we try to place all .binds() on the upper-most level, when it's possible, to make the whole picture easier to see?

test/clang-tidy/misc-dangling-handle.cpp
85 ↗(On Diff #49629)

This is intentional. It makes sense to specify the whole message once and remove repeated static text from other CHECK-MESSAGES lines to make them shorter and the whole test more readable.

sbenza updated this revision to Diff 49998.Mar 7 2016, 2:50 PM
sbenza marked 3 inline comments as done.
sbenza edited edge metadata.

Minor fixes

clang-tidy/misc/DanglingHandleCheck.cpp
24 ↗(On Diff #49629)

It is not the same.
That one is filtering the characters using isAlphanumeric.
Type names can have non-alpha chars as part of template instantiations.

On the other hand, it is a copy of the one from FasterStringFindCheck.
We could move it to a central location.

57 ↗(On Diff #49629)

Yes. hasName() and hasAnyName() have been fixed to look through inline namespaces.

116 ↗(On Diff #49629)

Will do.

test/clang-tidy/misc-dangling-handle.cpp
73 ↗(On Diff #49629)

Removed. Not needed here.

85 ↗(On Diff #49629)

They are truncated on purpose.
We use the full message in the first one to check the message.
The rest are truncated to fit in 80 chars.

jbcoe added inline comments.Mar 10 2016, 3:59 PM
test/clang-tidy/misc-dangling-handle.cpp
86 ↗(On Diff #49998)

Thanks.

alexfh accepted this revision.Mar 10 2016, 5:17 PM
alexfh edited edge metadata.

LG. Thank you for one more awesome check!

clang-tidy/misc/DanglingHandleCheck.cpp
25 ↗(On Diff #49998)

That one is filtering the characters using isAlphanumeric.

Maybe it shouldn't ;)

Anyways, we have to converge all these, but it's not important to do it in this patch.

117 ↗(On Diff #49998)

It's fine for a follow up, if it's more convenient to you.

This revision is now accepted and ready to land.Mar 10 2016, 5:17 PM
jbcoe accepted this revision.Mar 13 2016, 2:53 PM
jbcoe edited edge metadata.
jbcoe added a comment.Mar 22 2016, 4:19 AM

Do you have commit access? I can apply this patch for you if not.

Do you have commit access? I can apply this patch for you if not.

He does have commit access.

Do you have commit access? I can apply this patch for you if not.

I do.
I am waiting on D18275 to fix the problem with using internal::HasNameMatcher directly.

sbenza updated this revision to Diff 51684.Mar 25 2016, 2:17 PM
sbenza edited edge metadata.

Using new public hasAnyName API.

Still LG. Thank you!

This revision was automatically updated to reflect the committed changes.

Can you add a synopsis of this new check to docs/ReleaseNotes.rst please?

sbenza added a subscriber: sbenza.Mar 30 2016, 11:57 AM

This is already being done in http://reviews.llvm.org/D18582