Page MenuHomePhabricator

[analyzer] CallDescriptionMap: Support CXXConstructExpr
Needs ReviewPublic

Authored by Charusso on May 24 2020, 9:35 PM.

Details

Reviewers
NoQ
Summary

-

Diff Detail

Event Timeline

Charusso created this revision.May 24 2020, 9:35 PM
Charusso marked an inline comment as done.
Charusso added inline comments.
clang/lib/StaticAnalyzer/Core/CallEvent.cpp
384

dyn_cast, whoops.

Charusso marked an inline comment as done.May 24 2020, 10:00 PM
Charusso added inline comments.
clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
176

Weird formatting.

NoQ added a comment.Jun 1 2020, 5:03 AM

This is very handy, thank you!

clang/lib/StaticAnalyzer/Core/CallEvent.cpp
388–391

In any case, i'd prefer ND->getNameAsString() here.

clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
175

Let's also test {"std", "basic_string", "basic_string"}.

214

What do you think about having a constructor override for CallDescription that accepts an OverloadedOperatorKind? That'll be faster and more type-safe than matching operator name as string.

Charusso updated this revision to Diff 268075.Jun 3 2020, 12:56 AM
Charusso marked 5 inline comments as done.
Charusso retitled this revision from [analyzer] CallDescriptionMap: Support CXXConstructExpr, CXXOperatorCallExpr to [analyzer] CallDescriptionMap: Support CXXConstructExpr.
Charusso added a reviewer: aaron.ballman.
  • Refactor.
clang/lib/StaticAnalyzer/Core/CallEvent.cpp
388–391

+1, but the API is that smart: assert(Name.isIdentifier() && "Name is not a simple identifier");. However, we have ND->getDeclName().getAsString() which is equivalent to dumping to the stream (literally) and handles unnamed declarations, my bad. I have learnt from @aaron.ballman dumping to a stream is safe and I believed this is the only solution.

@aaron.ballman, what if we introduce getNameMayUnnamed() for convenient use? It would immediately ring the bells to use the right API.

clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
214

Great idea, thanks! Added in: D81059

Charusso marked an inline comment as not done.Jun 3 2020, 1:00 AM
NoQ added inline comments.Jun 3 2020, 4:21 AM
clang/lib/StaticAnalyzer/Core/CallEvent.cpp
388–391

assert(Name.isIdentifier() && "Name is not a simple identifier");

It's in getName(), not in getNameAsString(). The latter always succeeds.

Charusso updated this revision to Diff 268153.Jun 3 2020, 5:21 AM
Charusso marked 3 inline comments as done.
  • Using getNameAsString().
Charusso added a subscriber: aaron.ballman.
Charusso added inline comments.
clang/lib/StaticAnalyzer/Core/CallEvent.cpp
388–391

Hm, I have miss read the API, whoops. It is being deprecated 11 years ago and I have believed it delegates the work on getName(). Also @aaron.ballman believed Out << *ND is so cool, meanwhile it is the same as getNameAsString(). So much mystery around that API, facepalm. Thanks!

NoQ added inline comments.Jul 2 2020, 8:35 AM
clang/lib/StaticAnalyzer/Core/CallEvent.cpp
366

Why did you suddenly want to remove this cast? CXXConstructorDecl is still a FunctionDecl.

Wait, why do we even need a patch for this? Didn't it already work out of the box previously?

387

I agree this && FD was unnecessary though :D