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

[Chandler Carruth]

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 <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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110223/5d3142df/attachment.html>