This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][UninitializedObjectChecker] Fixed captured lambda variable name
ClosedPublic

Authored by Szelethus on Jun 18 2018, 11:02 AM.

Diff Detail

Repository
rC Clang

Event Timeline

Szelethus created this revision.Jun 18 2018, 11:02 AM
george.karpenkov requested changes to this revision.Jun 18 2018, 11:12 AM

Looks good overall, but some nits inline.

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
667

Can we have a comment explaining the semantics? From my understanding, you find a corresponding capture using a source location, and get a name from there.

670

CXXParent is guaranteed to be non-null at this stage, otherwise dyn_cast fails

672

Could we just use a for-each here? Sorry for being opinionated, I'm pretty sure it would be both shorter and more readable.

674

That's exactly the semantics of the equality operator on source locations, so why not Field->getLocation() == L.getLocation() ?

This revision now requires changes to proceed.Jun 18 2018, 11:12 AM
Szelethus added a comment.EditedJun 19 2018, 2:51 AM

Thanks for the review!

I spend some time thinking about the support for lambda functions, and I start to think that checking for lambda misuse shouldn't be the responsibility of this checker. I created a new revision for that discussion, I wouldn't like to abandon this one until I heard what you think of this! :)
D48318

edit: I was wrong on this one, sorry about that, so please just ignore it. I'll update this diff tomorrow.

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
672

Great idea! :) I completely agree, to be honest I scratched my head too while I wrote it down.

Szelethus added inline comments.Jun 19 2018, 8:59 AM
lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
670

I found this on http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates:

dyn_cast<>:

The dyn_cast<> operator is a “checking cast” operation. It checks to see if the operand is of the specified type, and if so, returns a pointer to it (this operator does not work with references). If the operand is not of the correct type, a null pointer is returned.

So I guess this should be alright.

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
670

Oops sorry my bad.

Szelethus updated this revision to Diff 152105.Jun 20 2018, 9:35 AM

Fixes according to inline comments.

Szelethus marked 7 inline comments as done.Jun 20 2018, 9:36 AM
NoQ added inline comments.Jun 26 2018, 5:12 PM
lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
675

Uhm, this source location comparison looks really flaky. Ideally we would have looked up by field and get the captured variable directly, but it seems that we don't have a ready-made method for this.

The second ideal thing would probably be a reverse variant of CXXRecordDecl::getCaptureFields(). Looking at how it's implemented, it seems that capture indix is in sync with field index. Because we have FieldDecl::getFieldIndex(), probably we could just lookup the captures array by that index?

Few more nits.

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
667

Is this a member method? Then it should be prefixed with a class name.
Currently it looks like you have a member method without an implementation and a separate C-style function?

668

Doxygen-style comments in LLVM start with a triple slash, and are located above the function. Then the tools for autocompletion (e.g. clangd) can pick them up.

george.karpenkov requested changes to this revision.Jun 26 2018, 5:16 PM

Looks like @NoQ had some valid comments as well.

This revision now requires changes to proceed.Jun 26 2018, 5:16 PM
Szelethus updated this revision to Diff 154420.Jul 6 2018, 8:54 AM

Finding the correct captured variable now works with FieldDecl::getFieldIndex().

Szelethus marked 3 inline comments as done.Jul 6 2018, 8:58 AM
Szelethus added inline comments.
lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
667

It is a statically defined method, and you can see its forward declaration in the same diff.

Back when I started writing this checker, the amount of function laying in the anonymous namespace was very few, but this has changed. I'll fix it sometime because it's getting somewhat annoying (and is against the coding standards).

NoQ accepted this revision.Jul 12 2018, 12:23 PM

Looks good!

lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
667

Yeah, just make them static, it's easier to read (https://llvm.org/docs/CodingStandards.html#anonymous-namespaces).

Szelethus updated this revision to Diff 155361.Jul 13 2018, 5:55 AM
Szelethus marked an inline comment as done.

Thanks! :)

Rebased to rC336994.

Szelethus marked 2 inline comments as done.Jul 13 2018, 5:55 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jul 13 2018, 6:00 AM
This revision was automatically updated to reflect the committed changes.