[PATCH] D139759: [analyzer] Fix assertion in getAPSIntType

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers marked 2 inline comments as done.
vabridgers added a comment.

Thanks Balazs, I think the comments have been addressed. Let me know if there's anything else to do, or if this is ready to land.

Best!



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:157-158
+           Loc::isLocType(T));
     return APSIntType(Ctx.getIntWidth(T),
                       !T->isSignedIntegerOrEnumerationType());
   }
----------------
steakhal wrote:
> I don't think you are supposed to call `isSignedIntegerOrEnumerationType()` if you have a //fixed-point// type.
> ```lang=C++
> inline bool Type::isSignedFixedPointType() const {
>   if (const auto *BT = dyn_cast<BuiltinType>(CanonicalType)) {
>     return ((BT->getKind() >= BuiltinType::ShortAccum &&
>              BT->getKind() <= BuiltinType::LongAccum) ||
>             (BT->getKind() >= BuiltinType::ShortFract &&
>              BT->getKind() <= BuiltinType::LongFract) ||
>             (BT->getKind() >= BuiltinType::SatShortAccum &&
>              BT->getKind() <= BuiltinType::SatLongAccum) ||
>             (BT->getKind() >= BuiltinType::SatShortFract &&
>              BT->getKind() <= BuiltinType::SatLongFract));
>   }
>   return false;
> }
> ```
> By looking at the implementation of this, I don't think you could substitute that with `isSignedIntegerOrEnumerationType()`.
> Am I wrong about this?
> 
> Please demonstrate this by tests.
I tried using isSignedIntegerOrEnumerationType() instead of (T->isIntegralOrEnumerationType() || T->isFixedPointType() ... ), but got the same assert :/  

I corrected the formatting and expanded the test cases. 


================
Comment at: clang/test/Analysis/fixed-point.c:6-9
+long a(int c) {                                                                       
+  (long _Accum) c >> 4;
+  return c;
+}  
----------------
steakhal wrote:
> It had a few extra spaces. And by casting it to void we could eliminate `-Wno-unused` as well.
Dang, I used git-clang-format to try cleaning the patch thinking it would clean up formatting, but I guess it didn't :/ 

I cleaned up the test case and removed the -Wno-unused option. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139759