Re: [cfe-commits] [Patch] -Wignored-qualifiers should point to the first ignored qualifier

[Hans Wennborg]

On Wed, Jun 1, 2011 at 9:20 PM, Douglas Gregor <dgre...@apple.com> wrote:
> This is looking really good. The only issue I see is when comparing source 
> locations with <, e.g.,
>
> +    if (!Loc.isValid() || ConstQualLoc < Loc)
> +      Loc = ConstQualLoc;
>
> The < operator on SourceLocation doesn't necessarily mean "before in the 
> translation"; it's a somewhat arbitrary total ordering useful mainly for use 
> in std::map or std::set.
>
> Instead, I suggest using SourceManager::isBeforeInTranslationUnit().

Thanks Doug! New patch attached.

Hans

Index: test/SemaCXX/return.cpp
===================================================================
--- test/SemaCXX/return.cpp	(revision 132456)
+++ test/SemaCXX/return.cpp	(working copy)
@@ -39,6 +39,11 @@
 char* const h(); // expected-warning{{'const' type qualifier on return type has no effect}}
 char* volatile i(); // expected-warning{{'volatile' type qualifier on return type has no effect}}
 
+char*
+volatile // expected-warning{{'const volatile' type qualifiers on return type have no effect}}
+const
+j();
+
 const volatile int scalar_cv(); // expected-warning{{'const volatile' type qualifiers on return type have no effect}}
 }
 
Index: lib/Sema/SemaType.cpp
===================================================================
--- lib/Sema/SemaType.cpp	(revision 132456)
+++ lib/Sema/SemaType.cpp	(working copy)
@@ -1479,41 +1479,37 @@
   FixItHint VolatileFixIt;
   FixItHint RestrictFixIt;
 
+  const SourceManager &SM = S.getSourceManager();
+
   // FIXME: The locations here are set kind of arbitrarily. It'd be nicer to
   // find a range and grow it to encompass all the qualifiers, regardless of
   // the order in which they textually appear.
   if (Quals & Qualifiers::Const) {
     ConstFixIt = FixItHint::CreateRemoval(ConstQualLoc);
-    Loc = ConstQualLoc;
-    ++NumQuals;
     QualStr = "const";
+    ++NumQuals;
+    if (!Loc.isValid() || SM.isBeforeInTranslationUnit(ConstQualLoc, Loc))
+      Loc = ConstQualLoc;
   }
   if (Quals & Qualifiers::Volatile) {
     VolatileFixIt = FixItHint::CreateRemoval(VolatileQualLoc);
-    if (NumQuals == 0) {
-      Loc = VolatileQualLoc;
-      QualStr = "volatile";
-    } else {
-      QualStr += " volatile";
-    }
+    QualStr += (NumQuals == 0 ? "volatile" : " volatile");
     ++NumQuals;
+    if (!Loc.isValid() || SM.isBeforeInTranslationUnit(VolatileQualLoc, Loc))
+      Loc = VolatileQualLoc;
   }
   if (Quals & Qualifiers::Restrict) {
     RestrictFixIt = FixItHint::CreateRemoval(RestrictQualLoc);
-    if (NumQuals == 0) {
-      Loc = RestrictQualLoc;
-      QualStr = "restrict";
-    } else {
-      QualStr += " restrict";
-    }
+    QualStr += (NumQuals == 0 ? "restrict" : " restrict");
     ++NumQuals;
+    if (!Loc.isValid() || SM.isBeforeInTranslationUnit(RestrictQualLoc, Loc))
+      Loc = RestrictQualLoc;
   }
 
   assert(NumQuals > 0 && "No known qualifiers?");
 
   S.Diag(Loc, diag::warn_qual_return_type)
-    << QualStr << NumQuals
-    << ConstFixIt << VolatileFixIt << RestrictFixIt;
+    << QualStr << NumQuals << ConstFixIt << VolatileFixIt << RestrictFixIt;
 }
 
 /// GetTypeForDeclarator - Convert the type for the specified

_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits