NoQ added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Core/DynamicType.cpp:73 + Ty = STTPTy->getReplacementType(); + if (Ty->isPointerType()) + Ty = Ty->getPointeeType(); ---------------- NoQ wrote: > baloghadamsoftware wrote: > > xazax.hun wrote: > > > Is this doing what you intended? What about a reference to a pointer? > > > Wouldn't you do too much unboxing? > > > > > > Also, I think a function returning a value would be more conventional. > > > > > > Other sugars like typedefs cannot interfere? I think this patch might > > > benefit from additional test coverage. I also see no tests for template > > > substitutions. > > Reference to pointer cast using //LLVM//'s cast functions are syntactically > > invalid, they do not compile. > > > > For `QualType` in-place modification is usual, since we use it by value. > > > > I see no test coverage for this particular part of the analyzer > > specifically, it seems that its is only tested indirectly in the tests for > > `CastValueChecker`. > The usual idiom is > ```lang=c++ > if (Ty->isPointerType() || Ty->isReferenceType()) > Ty = Ty->getPointeeType(); > ``` > It makes sure that you don't unwrap things twice. Also there's no need to > canonicalize before unwrapping the pointee type; all these methods > automatically operate over the canonical type. > > You might want to add tests for typedefs. Oof, this function is actually being fed references-to-pointers. I'll add a fixme. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85752/new/ https://reviews.llvm.org/D85752 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
