leonardchan added a comment.
@erichkeane I created a patch at https://reviews.llvm.org/D49327 with the fix
for this and added you as a reviewer.
Repository:
rC Clang
https://reviews.llvm.org/D46915
___
cfe-commits mailing list
cfe-commits@lists.
erichkeane added a comment.
In https://reviews.llvm.org/D46915#1162095, @leonardchan wrote:
> In https://reviews.llvm.org/D46915#1162038, @erichkeane wrote:
>
> > See this bug here: https://bugs.llvm.org/show_bug.cgi?id=38161
> >
> > This patch doesn't seem to properly consider integers, and igno
leonardchan added a comment.
In https://reviews.llvm.org/D46915#1162038, @erichkeane wrote:
> See this bug here: https://bugs.llvm.org/show_bug.cgi?id=38161
>
> This patch doesn't seem to properly consider integers, and ignores octals,
> which I suspect shouldn't be the case.
I just requested
erichkeane added a comment.
See this bug here: https://bugs.llvm.org/show_bug.cgi?id=38161
This patch doesn't seem to properly consider integers, and ignores octals,
which I suspect shouldn't be the case.
Repository:
rC Clang
https://reviews.llvm.org/D46915
__
leonardchan added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:90
+ // by default.
+ bool SameFBits;
+
rjmccall wrote:
> Sorry for the extremely late review, but this really needs to be renamed.
> Please remember that other compiler maintaine
rjmccall added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:90
+ // by default.
+ bool SameFBits;
+
Sorry for the extremely late review, but this really needs to be renamed.
Please remember that other compiler maintainers are not experts in t
This revision was automatically updated to reflect the committed changes.
leonardchan marked an inline comment as done.
Closed by commit rC335148: [Fixed Point Arithmetic] Fixed Point Precision Bits
and Fixed Point Literals (authored by leonardchan, committed by ).
Repository:
rC Clang
https:/
ebevhan accepted this revision.
ebevhan added inline comments.
This revision is now accepted and ready to land.
Comment at: lib/Sema/SemaExpr.cpp:1248
+ bool RHSFixed = RHSType->isFixedPointType();
+
+ if (LHSFixed && RHSFixed) {
leonardchan wrote:
> ebevhan wr
leonardchan updated this revision to Diff 151919.
leonardchan marked 2 inline comments as done.
Repository:
rC Clang
https://reviews.llvm.org/D46915
Files:
include/clang-c/Index.h
include/clang/AST/ASTContext.h
include/clang/AST/Expr.h
include/clang/AST/RecursiveASTVisitor.h
include/
leonardchan marked an inline comment as done.
leonardchan added inline comments.
Comment at: lib/Sema/SemaExpr.cpp:1248
+ bool RHSFixed = RHSType->isFixedPointType();
+
+ if (LHSFixed && RHSFixed) {
ebevhan wrote:
> leonardchan wrote:
> > leonardchan wrote:
> >
ebevhan added a comment.
Just a couple more comments and then I think it looks good.
We can discuss the conversion and comparison issues in later patches.
Comment at: include/clang/AST/ASTContext.h:1951
+ unsigned char getFixedPointScale(const QualType &Ty) const;
+ unsigned
leonardchan added inline comments.
Comment at: lib/Sema/SemaExpr.cpp:1248
+ bool RHSFixed = RHSType->isFixedPointType();
+
+ if (LHSFixed && RHSFixed) {
ebevhan wrote:
> ebevhan wrote:
> > leonardchan wrote:
> > > leonardchan wrote:
> > > > ebevhan wrote:
> > >
leonardchan updated this revision to Diff 151744.
leonardchan added a comment.
- Removed CK_IntegralToFixedPoint to be added to later patch addressing
conversion
- Create CXString for FixedPointLiteral
Repository:
rC Clang
https://reviews.llvm.org/D46915
Files:
include/clang-c/Index.h
i
ebevhan added inline comments.
Comment at: lib/Sema/SemaExpr.cpp:1248
+ bool RHSFixed = RHSType->isFixedPointType();
+
+ if (LHSFixed && RHSFixed) {
leonardchan wrote:
> leonardchan wrote:
> > ebevhan wrote:
> > > ebevhan wrote:
> > > > leonardchan wrote:
> > >
leonardchan updated this revision to Diff 151433.
leonardchan added a comment.
- Removed the functions for performing casting between fixed point types to a
common type when performing a binary operation since operations should be
performed on the types as is. Also this patch is meant more for d
leonardchan updated this revision to Diff 151431.
leonardchan added a comment.
Removed FractFBits property. Fractional bits for _Fract types are now one less
the _Fract width unless `SameFBits` is specified. In that case, the number of
FBits in unsigned _Fract is the same as that of signed _Frac
leonardchan added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:89
+ // corresponding unsaturated types.
+ unsigned char ShortAccumFBits, ShortAccumIBits;
+ unsigned char AccumFBits, AccumIBits;
ebevhan wrote:
> leonardchan wrote:
> > ebevhan w
ebevhan added inline comments.
Comment at: lib/Sema/SemaExpr.cpp:1248
+ bool RHSFixed = RHSType->isFixedPointType();
+
+ if (LHSFixed && RHSFixed) {
ebevhan wrote:
> leonardchan wrote:
> > ebevhan wrote:
> > > leonardchan wrote:
> > > > ebevhan wrote:
> > > > >
ebevhan added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:89
+ // corresponding unsaturated types.
+ unsigned char ShortAccumFBits, ShortAccumIBits;
+ unsigned char AccumFBits, AccumIBits;
leonardchan wrote:
> ebevhan wrote:
> > leonardchan w
leonardchan added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:89
+ // corresponding unsaturated types.
+ unsigned char ShortAccumFBits, ShortAccumIBits;
+ unsigned char AccumFBits, AccumIBits;
ebevhan wrote:
> leonardchan wrote:
> > ebevhan w
ebevhan added inline comments.
Comment at: lib/AST/ExprConstant.cpp:9437
+ }
+ return Success(-Value, E);
+}
This looks very suspect to me as well... This might not respect padding on
types whose sign bit is not the MSB, such as _Fract. The same go
ebevhan added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:89
+ // corresponding unsaturated types.
+ unsigned char ShortAccumFBits, ShortAccumIBits;
+ unsigned char AccumFBits, AccumIBits;
leonardchan wrote:
> ebevhan wrote:
> > leonardchan w
ebevhan added inline comments.
Comment at: lib/Lex/LiteralSupport.cpp:1187
+
+ uint64_t Base = (radix == 16) ? 2 : 10;
+ if (BaseShift > 0) {
leonardchan wrote:
> ebevhan wrote:
> > I don't think loops are needed here. BaseShift is essentially Exponent so
> >
leonardchan added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:89
+ // corresponding unsaturated types.
+ unsigned char ShortAccumFBits, ShortAccumIBits;
+ unsigned char AccumFBits, AccumIBits;
ebevhan wrote:
> leonardchan wrote:
> > ebevhan w
leonardchan updated this revision to Diff 151016.
leonardchan marked 9 inline comments as done.
Repository:
rC Clang
https://reviews.llvm.org/D46915
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Expr.h
include/clang/AST/OperationKinds.def
include/clang/AST/RecursiveASTVisitor
ebevhan added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:89
+ // corresponding unsaturated types.
+ unsigned char ShortAccumFBits, ShortAccumIBits;
+ unsigned char AccumFBits, AccumIBits;
leonardchan wrote:
> ebevhan wrote:
> > I suspect it'
leonardchan added inline comments.
Comment at: include/clang/AST/Type.h:6552
+// as a scaled integer.
+std::string FixedPointValueToString(unsigned Radix, unsigned Scale,
+uint64_t Val);
ebevhan wrote:
> This should probably ta
leonardchan updated this revision to Diff 150602.
leonardchan marked 14 inline comments as done.
Repository:
rC Clang
https://reviews.llvm.org/D46915
Files:
include/clang/AST/ASTContext.h
include/clang/AST/Expr.h
include/clang/AST/OperationKinds.def
include/clang/AST/RecursiveASTVisito
ebevhan added inline comments.
Comment at: include/clang/AST/Type.h:6552
+// as a scaled integer.
+std::string FixedPointValueToString(unsigned Radix, unsigned Scale,
+uint64_t Val);
This should probably take an APInt or APSInt
leonardchan added inline comments.
Comment at: lib/AST/ExprConstant.cpp:9323
+ if (Value.isSigned() && Value.isMinSignedValue() && E->canOverflow() &&
+ !HandleOverflow(Info, E, -Value.extend(Value.getBitWidth() + 1),
+ E->getType()))
--
30 matches
Mail list logo
