Thank you both for the input!
On Wed, Feb 23, 2011 at 6:00 PM, Douglas Gregor <[email protected]> wrote:
>
> On Feb 23, 2011, at 9:23 AM, Chandler Carruth wrote:
>
>> One overall concern I would have is that a parser struct is quadrupling in
>> size. Can you measure the peak memory usage impact of this change on a hefty
>> compilation?
>
> It shouldn't have any effect whatsoever, since PointerTypeInfo is only used
> when it's in a union with the much-larger FunctionTypeInfo.
Great, I won't worry about that then.
>> Some nits below...
>>
>> Index: test/SemaCXX/return.cpp
>> ===================================================================
>> --- test/SemaCXX/return.cpp (revision 126218)
>> +++ test/SemaCXX/return.cpp (working copy)
>> @@ -24,5 +24,14 @@
>> const volatile S class_cv();
>>
>> const int scalar_c(); // expected-warning{{'const' type qualifier on return
>> type has no effect}}
>> +
>> +const
>> +char*
>> +const // expected-warning{{'const' type qualifier on return type has no
>> effect}}
>> +f();
>>
>> It would be good to test "T const" and "T const * const" as well for
>> completeness...
Adding that.
>>
>> Index: include/clang/Sema/DeclSpec.h
>> ===================================================================
>> --- include/clang/Sema/DeclSpec.h (revision 126218)
>> +++ include/clang/Sema/DeclSpec.h (working copy)
>> @@ -825,6 +825,25 @@
>> struct PointerTypeInfo : TypeInfoCommon {
>> /// The type qualifiers: const/volatile/restrict.
>> unsigned TypeQuals : 3;
>> +
>> + /// The location of the const-qualifier, if any.
>> + unsigned ConstQualLoc;
>> +
>> + /// The location of the volatile-qualifier, if any.
>> + unsigned VolatileQualLoc;
>> +
>> + /// The location of the restrict-qualifier, if any.
>> + unsigned RestrictQualLoc;
>> +
>> + SourceLocation constQualLoc() const {
>> + return SourceLocation::getFromRawEncoding(ConstQualLoc);
>> + }
>> + SourceLocation volatileQualLoc() const {
>> + return SourceLocation::getFromRawEncoding(VolatileQualLoc);
>> + }
>> + SourceLocation restrictQualLoc() const {
>> + return SourceLocation::getFromRawEncoding(RestrictQualLoc);
>> + }
>>
>> I find the getters here a bit odd. As these are only accessed on one place,
>> I wonder if we should do the raw encoding -> SourceLocation transition there.
You're right. I'll remove the getters.
>>
>> Perhaps most odd to me is that the members are public and doxymented, but
>> they have non-trivial getters and those aren't doxymented. If you want
>> getters on this struct, I'd make the members private and move the comments.
>>
>> Index: lib/Sema/SemaType.cpp
>> ===================================================================
>> --- lib/Sema/SemaType.cpp (revision 126218)
>> +++ lib/Sema/SemaType.cpp (working copy)
>> @@ -1395,6 +1395,49 @@
>> return QT;
>> }
>>
>> +static void DiagnoseIgnoredQualifiers(unsigned Quals,
>> + SourceLocation constQualLoc,
>> + SourceLocation volatileQualLoc,
>> + SourceLocation restrictQualLoc,
>> + Sema& S) {
>> + std::string QualStr;
>> + unsigned NumQuals = 0;
>> + SourceLocation Loc;
>> +
>> + if (Quals & Qualifiers::Const) {
>> + Loc = constQualLoc;
>> + ++NumQuals;
>> + QualStr = "const";
>> + }
>> + if (Quals & Qualifiers::Volatile) {
>> + if (NumQuals == 0) {
>> + Loc = volatileQualLoc;
>> + QualStr = "volatile";
>> + } else
>> + QualStr += " volatile";
>>
>> I think this section is moved verbatim, but while we're here, can we have
>> {}s around this else block? Especially followed by a preincrement, I had to
>> read in 3 times...
Done.
>>
>> + ++NumQuals;
>> + }
>> + if (Quals & Qualifiers::Restrict) {
>> + if (NumQuals == 0) {
>> + Loc = restrictQualLoc;
>> + QualStr = "restrict";
>> + } else
>> + QualStr += " restrict";
>>
>> Same here.
Done.
>>
>> + ++NumQuals;
>> + }
>> +
>> + assert(NumQuals > 0 && "No known qualifiers?");
>> + Sema::SemaDiagnosticBuilder DB = S.Diag(Loc, diag::warn_qual_return_type);
>> +
>> + DB << QualStr << NumQuals;
>> + if (Quals & Qualifiers::Const)
>> + DB << FixItHint::CreateRemoval(constQualLoc);
>> + if (Quals & Qualifiers::Volatile)
>> + DB << FixItHint::CreateRemoval(volatileQualLoc);
>> + if (Quals & Qualifiers::Restrict)
>> + DB << FixItHint::CreateRemoval(restrictQualLoc);
>>
>> And I think we simplify this by having three empty FixItHint variable, and
>> initializing them in each branch where we do the original checking. That
>> should also avoid the need to store the diagnostic builder, and we can just
>> stream the lot of this in directly.
Ah, yes that makes it nicer.
Attaching new patch addressing Chandler's comments.
Index: test/Sema/return.c
===================================================================
--- test/Sema/return.c (revision 126218)
+++ test/Sema/return.c (working copy)
@@ -242,6 +242,7 @@
// Test warnings on ignored qualifiers on return types.
const int ignored_c_quals(); // expected-warning{{'const' type qualifier on return type has no effect}}
const volatile int ignored_cv_quals(); // expected-warning{{'const volatile' type qualifiers on return type have no effect}}
+char* const volatile restrict ignored_cvr_quals(); // expected-warning{{'const volatile restrict' type qualifiers on return type have no effect}}
// Test that for switch(enum) that if the switch statement covers all the cases
// that we don't consider that for -Wreturn-type.
@@ -254,4 +255,3 @@
case C3: return 4;
}
} // no-warning
-
Index: test/SemaCXX/return.cpp
===================================================================
--- test/SemaCXX/return.cpp (revision 126218)
+++ test/SemaCXX/return.cpp (working copy)
@@ -24,5 +24,20 @@
const volatile S class_cv();
const int scalar_c(); // expected-warning{{'const' type qualifier on return type has no effect}}
+int const scalar_c2(); // expected-warning{{'const' type qualifier on return type has no effect}}
+
+const
+char*
+const // expected-warning{{'const' type qualifier on return type has no effect}}
+f();
+
+char
+const*
+const // expected-warning{{'const' type qualifier on return type has no effect}}
+g();
+
+char* const h(); // expected-warning{{'const' type qualifier on return type has no effect}}
+char* volatile i(); // expected-warning{{'volatile' type qualifier on return type has no effect}}
+
const volatile int scalar_cv(); // expected-warning{{'const volatile' type qualifiers on return type have no effect}}
}
Index: include/clang/Sema/DeclSpec.h
===================================================================
--- include/clang/Sema/DeclSpec.h (revision 126218)
+++ include/clang/Sema/DeclSpec.h (working copy)
@@ -825,6 +825,16 @@
struct PointerTypeInfo : TypeInfoCommon {
/// The type qualifiers: const/volatile/restrict.
unsigned TypeQuals : 3;
+
+ /// The location of the const-qualifier, if any.
+ unsigned ConstQualLoc;
+
+ /// The location of the volatile-qualifier, if any.
+ unsigned VolatileQualLoc;
+
+ /// The location of the restrict-qualifier, if any.
+ unsigned RestrictQualLoc;
+
void destroy() {
}
};
@@ -1055,12 +1065,18 @@
/// getPointer - Return a DeclaratorChunk for a pointer.
///
static DeclaratorChunk getPointer(unsigned TypeQuals, SourceLocation Loc,
+ SourceLocation ConstQualLoc,
+ SourceLocation VolatileQualLoc,
+ SourceLocation RestrictQualLoc,
const ParsedAttributes &attrs) {
DeclaratorChunk I;
- I.Kind = Pointer;
- I.Loc = Loc;
- I.Ptr.TypeQuals = TypeQuals;
- I.Ptr.AttrList = attrs.getList();
+ I.Kind = Pointer;
+ I.Loc = Loc;
+ I.Ptr.TypeQuals = TypeQuals;
+ I.Ptr.ConstQualLoc = ConstQualLoc.getRawEncoding();
+ I.Ptr.VolatileQualLoc = VolatileQualLoc.getRawEncoding();
+ I.Ptr.RestrictQualLoc = RestrictQualLoc.getRawEncoding();
+ I.Ptr.AttrList = attrs.getList();
return I;
}
Index: lib/Sema/SemaType.cpp
===================================================================
--- lib/Sema/SemaType.cpp (revision 126218)
+++ lib/Sema/SemaType.cpp (working copy)
@@ -1395,6 +1395,54 @@
return QT;
}
+static void DiagnoseIgnoredQualifiers(unsigned Quals,
+ SourceLocation ConstQualLoc,
+ SourceLocation VolatileQualLoc,
+ SourceLocation RestrictQualLoc,
+ Sema& S) {
+ std::string QualStr;
+ unsigned NumQuals = 0;
+ SourceLocation Loc;
+
+ FixItHint ConstFixIt;
+ FixItHint VolatileFixIt;
+ FixItHint RestrictFixIt;
+
+ if (Quals & Qualifiers::Const) {
+ ConstFixIt = FixItHint::CreateRemoval(ConstQualLoc);
+ Loc = ConstQualLoc;
+ ++NumQuals;
+ QualStr = "const";
+ }
+ if (Quals & Qualifiers::Volatile) {
+ VolatileFixIt = FixItHint::CreateRemoval(VolatileQualLoc);
+ if (NumQuals == 0) {
+ Loc = VolatileQualLoc;
+ QualStr = "volatile";
+ } else {
+ QualStr += " volatile";
+ }
+ ++NumQuals;
+ }
+ if (Quals & Qualifiers::Restrict) {
+ RestrictFixIt = FixItHint::CreateRemoval(RestrictQualLoc);
+ if (NumQuals == 0) {
+ Loc = RestrictQualLoc;
+ QualStr = "restrict";
+ } else {
+ QualStr += " restrict";
+ }
+ ++NumQuals;
+ }
+
+ assert(NumQuals > 0 && "No known qualifiers?");
+
+ S.Diag(Loc, diag::warn_qual_return_type) << QualStr << NumQuals
+ << ConstFixIt
+ << VolatileFixIt
+ << RestrictFixIt;
+}
+
/// GetTypeForDeclarator - Convert the type for the specified
/// declarator to Type instances.
///
@@ -1678,46 +1726,31 @@
// cv-qualifiers on return types are pointless except when the type is a
// class type in C++.
- if (T.getCVRQualifiers() && D.getDeclSpec().getTypeQualifiers() &&
+ if (T->isPointerType() && T.getCVRQualifiers() &&
+ (!getLangOptions().CPlusPlus || !T->isDependentType())) {
+ assert(chunkIndex + 1 < e && "No DeclaratorChunk for the return type?");
+ DeclaratorChunk ReturnTypeChunk = D.getTypeObject(chunkIndex + 1);
+ assert(ReturnTypeChunk.Kind == DeclaratorChunk::Pointer);
+
+ DeclaratorChunk::PointerTypeInfo &PTI = ReturnTypeChunk.Ptr;
+
+ DiagnoseIgnoredQualifiers(PTI.TypeQuals,
+ SourceLocation::getFromRawEncoding(PTI.ConstQualLoc),
+ SourceLocation::getFromRawEncoding(PTI.VolatileQualLoc),
+ SourceLocation::getFromRawEncoding(PTI.RestrictQualLoc),
+ *this);
+
+ } else if (T.getCVRQualifiers() && D.getDeclSpec().getTypeQualifiers() &&
(!getLangOptions().CPlusPlus ||
(!T->isDependentType() && !T->isRecordType()))) {
- unsigned Quals = D.getDeclSpec().getTypeQualifiers();
- std::string QualStr;
- unsigned NumQuals = 0;
- SourceLocation Loc;
- if (Quals & Qualifiers::Const) {
- Loc = D.getDeclSpec().getConstSpecLoc();
- ++NumQuals;
- QualStr = "const";
- }
- if (Quals & Qualifiers::Volatile) {
- if (NumQuals == 0) {
- Loc = D.getDeclSpec().getVolatileSpecLoc();
- QualStr = "volatile";
- } else
- QualStr += " volatile";
- ++NumQuals;
- }
- if (Quals & Qualifiers::Restrict) {
- if (NumQuals == 0) {
- Loc = D.getDeclSpec().getRestrictSpecLoc();
- QualStr = "restrict";
- } else
- QualStr += " restrict";
- ++NumQuals;
- }
- assert(NumQuals > 0 && "No known qualifiers?");
-
- SemaDiagnosticBuilder DB = Diag(Loc, diag::warn_qual_return_type);
- DB << QualStr << NumQuals;
- if (Quals & Qualifiers::Const)
- DB << FixItHint::CreateRemoval(D.getDeclSpec().getConstSpecLoc());
- if (Quals & Qualifiers::Volatile)
- DB << FixItHint::CreateRemoval(D.getDeclSpec().getVolatileSpecLoc());
- if (Quals & Qualifiers::Restrict)
- DB << FixItHint::CreateRemoval(D.getDeclSpec().getRestrictSpecLoc());
+
+ DiagnoseIgnoredQualifiers(D.getDeclSpec().getTypeQualifiers(),
+ D.getDeclSpec().getConstSpecLoc(),
+ D.getDeclSpec().getVolatileSpecLoc(),
+ D.getDeclSpec().getRestrictSpecLoc(),
+ *this);
}
-
+
if (getLangOptions().CPlusPlus && D.getDeclSpec().isTypeSpecOwned()) {
// C++ [dcl.fct]p6:
// Types shall not be defined in return or parameter types.
Index: lib/Parse/ParseDecl.cpp
===================================================================
--- lib/Parse/ParseDecl.cpp (revision 126218)
+++ lib/Parse/ParseDecl.cpp (working copy)
@@ -2746,6 +2746,9 @@
if (Kind == tok::star)
// Remember that we parsed a pointer type, and remember the type-quals.
D.AddTypeInfo(DeclaratorChunk::getPointer(DS.getTypeQualifiers(), Loc,
+ DS.getConstSpecLoc(),
+ DS.getVolatileSpecLoc(),
+ DS.getRestrictSpecLoc(),
DS.takeAttributes()),
SourceLocation());
else
@@ -3742,4 +3745,3 @@
}
return false;
}
-
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
