This looks fine to me, do you have commit access, or shall I commit for you?<br><br><div class="gmail_quote">On Wed, Feb 23, 2011 at 10:17 AM, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org">hans@chromium.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">Thank you both for the input!<br>
<div class="im"><br>
On Wed, Feb 23, 2011 at 6:00 PM, Douglas Gregor <<a href="mailto:dgregor@apple.com">dgregor@apple.com</a>> wrote:<br>
><br>
> On Feb 23, 2011, at 9:23 AM, Chandler Carruth wrote:<br>
><br>
>> 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?<br>
><br>
> It shouldn't have any effect whatsoever, since PointerTypeInfo is only used when it's in a union with the much-larger FunctionTypeInfo.<br>
</div>Great, I won't worry about that then.<br>
<div class="im"><br>
>> Some nits below...<br>
>><br>
>> Index: test/SemaCXX/return.cpp<br>
>> ===================================================================<br>
>> --- test/SemaCXX/return.cpp (revision 126218)<br>
>> +++ test/SemaCXX/return.cpp (working copy)<br>
>> @@ -24,5 +24,14 @@<br>
>> const volatile S class_cv();<br>
>><br>
>> const int scalar_c(); // expected-warning{{'const' type qualifier on return type has no effect}}<br>
>> +<br>
>> +const<br>
>> +char*<br>
>> +const // expected-warning{{'const' type qualifier on return type has no effect}}<br>
>> +f();<br>
>><br>
>> It would be good to test "T const" and "T const * const" as well for completeness...<br>
</div>Adding that.<br>
<div class="im"><br>
>><br>
>> Index: include/clang/Sema/DeclSpec.h<br>
>> ===================================================================<br>
>> --- include/clang/Sema/DeclSpec.h (revision 126218)<br>
>> +++ include/clang/Sema/DeclSpec.h (working copy)<br>
>> @@ -825,6 +825,25 @@<br>
>> struct PointerTypeInfo : TypeInfoCommon {<br>
>> /// The type qualifiers: const/volatile/restrict.<br>
>> unsigned TypeQuals : 3;<br>
>> +<br>
>> + /// The location of the const-qualifier, if any.<br>
>> + unsigned ConstQualLoc;<br>
>> +<br>
>> + /// The location of the volatile-qualifier, if any.<br>
>> + unsigned VolatileQualLoc;<br>
>> +<br>
>> + /// The location of the restrict-qualifier, if any.<br>
>> + unsigned RestrictQualLoc;<br>
>> +<br>
>> + SourceLocation constQualLoc() const {<br>
>> + return SourceLocation::getFromRawEncoding(ConstQualLoc);<br>
>> + }<br>
>> + SourceLocation volatileQualLoc() const {<br>
>> + return SourceLocation::getFromRawEncoding(VolatileQualLoc);<br>
>> + }<br>
>> + SourceLocation restrictQualLoc() const {<br>
>> + return SourceLocation::getFromRawEncoding(RestrictQualLoc);<br>
>> + }<br>
>><br>
>> 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.<br>
</div>You're right. I'll remove the getters.<br>
<div><div></div><div class="h5"><br>
>><br>
>> 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.<br>
>><br>
>> Index: lib/Sema/SemaType.cpp<br>
>> ===================================================================<br>
>> --- lib/Sema/SemaType.cpp (revision 126218)<br>
>> +++ lib/Sema/SemaType.cpp (working copy)<br>
>> @@ -1395,6 +1395,49 @@<br>
>> return QT;<br>
>> }<br>
>><br>
>> +static void DiagnoseIgnoredQualifiers(unsigned Quals,<br>
>> + SourceLocation constQualLoc,<br>
>> + SourceLocation volatileQualLoc,<br>
>> + SourceLocation restrictQualLoc,<br>
>> + Sema& S) {<br>
>> + std::string QualStr;<br>
>> + unsigned NumQuals = 0;<br>
>> + SourceLocation Loc;<br>
>> +<br>
>> + if (Quals & Qualifiers::Const) {<br>
>> + Loc = constQualLoc;<br>
>> + ++NumQuals;<br>
>> + QualStr = "const";<br>
>> + }<br>
>> + if (Quals & Qualifiers::Volatile) {<br>
>> + if (NumQuals == 0) {<br>
>> + Loc = volatileQualLoc;<br>
>> + QualStr = "volatile";<br>
>> + } else<br>
>> + QualStr += " volatile";<br>
>><br>
>> 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...<br>
</div></div>Done.<br>
<div class="im"><br>
>><br>
>> + ++NumQuals;<br>
>> + }<br>
>> + if (Quals & Qualifiers::Restrict) {<br>
>> + if (NumQuals == 0) {<br>
>> + Loc = restrictQualLoc;<br>
>> + QualStr = "restrict";<br>
>> + } else<br>
>> + QualStr += " restrict";<br>
>><br>
>> Same here.<br>
</div>Done.<br>
<div class="im"><br>
>><br>
>> + ++NumQuals;<br>
>> + }<br>
>> +<br>
>> + assert(NumQuals > 0 && "No known qualifiers?");<br>
>> + Sema::SemaDiagnosticBuilder DB = S.Diag(Loc, diag::warn_qual_return_type);<br>
>> +<br>
>> + DB << QualStr << NumQuals;<br>
>> + if (Quals & Qualifiers::Const)<br>
>> + DB << FixItHint::CreateRemoval(constQualLoc);<br>
>> + if (Quals & Qualifiers::Volatile)<br>
>> + DB << FixItHint::CreateRemoval(volatileQualLoc);<br>
>> + if (Quals & Qualifiers::Restrict)<br>
>> + DB << FixItHint::CreateRemoval(restrictQualLoc);<br>
>><br>
>> 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.<br>
</div>Ah, yes that makes it nicer.<br>
<br>
<br>
Attaching new patch addressing Chandler's comments.<br>
</blockquote></div><br>