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

[Chandler Carruth]

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?

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...

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.

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...

+    ++NumQuals;
+  }
+  if (Quals & Qualifiers::Restrict) {
+    if (NumQuals == 0) {
+      Loc = restrictQualLoc;
+      QualStr = "restrict";
+    } else
+      QualStr += " restrict";

Same here.

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