[cfe-commits] [Patch] Fix -Wignored-qualifiers

[Hans Wennborg]

On Wed, Feb 23, 2011 at 6:30 PM, Chandler Carruth <chandlerc at google.com> 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 <hans at chromium.org> wrote:
>>
>> Thank you both for the input!
>>
>> On Wed, Feb 23, 2011 at 6:00 PM, Douglas Gregor <dgregor at apple.com> 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.
>
>