This looks fine to me, do you have commit access, or shall I commit for you?
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
