[PATCH] D85752: [Analyzer] Store the pointed/referenced type for dynamic casts

Artem Dergachev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Aug 27 11:43:09 PDT 2020


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



More information about the cfe-commits mailing list