ASDenysPetrov added inline comments.

================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:200-204
+    } else {
+      //    [ First ]------>
+      //    [ Second  ]--->
+      // MIN^
+      // The First range is entirely inside the Second one.
----------------
steakhal wrote:
> Might be `First->To() == Second->To()`. In this case the comment is not 
> completely accurate.
I'll update.


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:81
   const llvm::APSInt &from(BaseType X) {
-    llvm::APSInt Dummy = Base;
-    Dummy = X;
-    return BVF.getValue(Dummy);
+    static llvm::APSInt Base{sizeof(BaseType) * 8,
+                             std::is_unsigned<BaseType>::value};
----------------
steakhal wrote:
> ASDenysPetrov wrote:
> > steakhal wrote:
> > > ASDenysPetrov wrote:
> > > > steakhal wrote:
> > > > > Shouldn't you use `sizeof(BaseType) * CHAR_BIT` instead?
> > > > Agree. It's better to avoid magic numbers. I'll fix.
> > > It's not only that but just imagine testing a clang on a special hardware 
> > > where they have let's say 9 bit bytes, for parity or something similar 
> > > stuff.
> > > The test would suddenly break.
> > > Although I suspect there would be many more things to break TBH xD
> > I am always skeptical about using`CHAR_BIT`, beacuse it represents bit 
> > number in `char`. And what if it would be 16 for instance (aka 2 bytes). 
> > But my intention is to get an amount of bits for a particular type. And I 
> > want something to represent a number of bits in a byte as a fundamental 
> > unit, but not something that depends on a `char` size on a particular 
> > platform.
> > I would better introduce something like `constexpr size_t BITS_IN_BYTE = 
> > 8;`.
> [[ https://eel.is/c++draft/basic.memobj#footnote-22 | basic.memobj 6.7.1/22 
> ]]:
> >The number of bits in a byte is reported by the macro `CHAR_­BIT` in the 
> >header `<climits>`.
These legacy names... :) I'll update.


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

https://reviews.llvm.org/D99797

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to