On Wed, Feb 23, 2011 at 6:30 PM, Chandler Carruth <[email protected]> wrote: > This looks fine to me, do you have commit access, or shall I commit for you? You do the honors, I don't have commit access.
Thanks, Hans > > On Wed, Feb 23, 2011 at 10:17 AM, Hans Wennborg <[email protected]> wrote: >> >> 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. > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
