[PATCH] D85728: [Analyzer] Support for the new variadic isa<> and isa_and_not_null<> in CastValueChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:289
+  for (QualType CastToTy: CastToTyVec) {
+    if (CastFromTy->isPointerType())
+      CastToTy = C.getASTContext().getPointerType(CastToTy);
----------------
Hmm, is this phabricator's way of displaying tabs?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:270-272
+    default:
+      llvm_unreachable("Invalid template argument for isa<> or "
+                       "isa_and_nonnull<>");
----------------
martong wrote:
> baloghadamsoftware wrote:
> > NoQ wrote:
> > > We shouldn't crash when code under analysis doesn't match our expectation.
> > The question is whether we can get any other template argument kind for `llvm::isa<>()` and `llvm::isa_and_nonnull<>()` than `Type` or `Pack`?
> I think it is worth to ponder about the below case. What if `isa` is used in a dependent context (i.e. it is used inside another template)?
> ```
>     /// The template argument is an expression, and we've not resolved it to one
>     /// of the other forms yet, either because it's dependent or because we're
>     /// representing a non-canonical template argument (for instance, in a
>     /// TemplateSpecializationType).
>     Expression,
> ```
Aha, yup, that's better, thanks.

> The question is whether we can get any other template argument kind for `llvm::isa<>()` and `llvm::isa_and_nonnull<>()` than Type or Pack?

It's a more general problem: what about a completely unrelated template function that is accidentally called `llvm::isa<>()` or `llvm::isa_and_nonnull<>()`? Or what if `llvm::isa<>()`'s/ `llvm::isa_and_nonnull<>()`'s implementation changes again? If it's at all possible in any template function, we should take it into account and at least provide an early return, we shouldn't rule out that possibility by looking at the name only.

> I think it is worth to ponder about the below case. What if `isa` is used in a dependent context (i.e. it is used inside another template)?

Ok, this one probably doesn't happen, because we don't ever try to symbolically execute templated ASTs (that aren't fully instantiated).


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

https://reviews.llvm.org/D85728