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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
