Okay, I fixed the concerns you raised. Is this better?
On Thu, Apr 28, 2011 at 2:32 PM, Richard Smith <[email protected]>wrote:
> Hi Richard,
>
> On Tue, April 26, 2011 22:49, Richard Trieu wrote:
> > In this case, the integer in APInt is 64 bits (returned from
> > CAT->getSize()), while the type given is sized as 32 bits
> (Context.IntTy).
> > A truncate instead of an extend is required. Is getSize() returning
> > with a larger bit width than necessary or can the array index exceed the
> > size of an int?
>
> The array bound can exceed the size of an int. However, it cannot exceed
> the size of a ptrdiff_t: see [support.types]p5.
>
Range-based for-loops now use ptrdiff_t for the type. This passes the new
bit length check in the integer literal constructor. Extending the integer
wasn't necessary, so I didn't add code for it.
>
>
> Also, if we do proceed this way, the new integer literal
> > creation method I wrote would not be used. Will it still be useful or
> > should I remove it?
>
> Best to avoid dead code. Your patch will still be in the archives if we
> want that code for anything.
>
Removed my unused code.
>
> Best regards,
> Richard
>
> > On Tue, Apr 26, 2011 at 5:50 AM, Richard Smith
> > <[email protected]>wrote:
> >> Hi Richard,
> >>
> >> Thanks for catching this, and my apologies for the delay in getting
> >> back to you.
> >>
> >> On Fri, 22 Apr 2011 21:26, Richard Trieu <[email protected]> wrote:
> >>
> >>> The code for range-based for-loops makes a bad integer literal
> >>> expression which has a mis-match between the size of the literal and
> >>> the integer type. This causes an assert to be thrown when
> >>> IsIntegerConstant() is
> >>> called on it. For this patch,
> >>>
> >>> 1) the assert that checks bit width is copied from
> >>> IsIntegerConstant()
> >>> into the integer literal constructor.
> >>
> >> This looks fine.
> >>
> >>
> >>> 2) a new static function was written so that it automatically selects
> >>>
> >> the > correct size integer and returns a proper integer literal
> >>>
> >>> 3) the range-based for-loops now use the function in #2 to get the
> >>> right integer literal
> >>
> >> I'd prefer for us to always use ptrdiff_t[*] for this (which is
> >> guaranteed to be big enough), and to extend the integer to fit, rather
> >> than picking a 'big enough' type based on the integer's value.
> >>
> >>
> >> [*] I think this actually needs to be the appropriate ptrdiff_t for the
> >> address space of the array.
> >>
> >> Thanks!
> >> Richard
>
>
>
Index: include/clang/AST/Expr.h
===================================================================
--- include/clang/AST/Expr.h (revision 129825)
+++ include/clang/AST/Expr.h (working copy)
@@ -977,13 +977,18 @@
false),
Loc(l) {
assert(type->isIntegerType() && "Illegal type in IntegerLiteral");
+ assert(V.getBitWidth() == C.getIntWidth(type) &&
+ "Integer type is not the correct size for constant.");
setValue(C, V);
}
- // type should be IntTy, LongTy, LongLongTy, UnsignedIntTy, UnsignedLongTy,
- // or UnsignedLongLongTy
+ /// \brief Returns a new integer literal with value 'V' and type 'type'.
+ /// \param type - either IntTy, LongTy, LongLongTy, UnsignedIntTy,
+ /// UnsignedLongTy, or UnsignedLongLongTy which should match the size of V
+ /// \param V - the value that the returned integer literal contains.
static IntegerLiteral *Create(ASTContext &C, const llvm::APInt &V,
QualType type, SourceLocation l);
+ /// \brief Returns a new empty interger literal.
static IntegerLiteral *Create(ASTContext &C, EmptyShell Empty);
llvm::APInt getValue() const { return Num.getValue(); }
Index: lib/Sema/SemaStmt.cpp
===================================================================
--- lib/Sema/SemaStmt.cpp (revision 129825)
+++ lib/Sema/SemaStmt.cpp (working copy)
@@ -1257,7 +1257,8 @@
ExprResult BoundExpr;
if (const ConstantArrayType *CAT = dyn_cast<ConstantArrayType>(UnqAT))
BoundExpr = Owned(IntegerLiteral::Create(Context, CAT->getSize(),
- Context.IntTy, RangeLoc));
+ Context.getPointerDiffType(),
+ RangeLoc));
else if (const VariableArrayType *VAT =
dyn_cast<VariableArrayType>(UnqAT))
BoundExpr = VAT->getSizeExpr();
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
