[PATCH] Fix UseAuto not transforming iterators when non-fully qualified names are used and libc++.

Guillaume Papin guillaume.papin at epitech.eu
Tue Jul 9 10:11:55 PDT 2013


  Some errors in the test cases themselves have been fixed but no test cases have been added to really test the `inline namespace`, I would suggest adding one (see comments).


================
Comment at: test/cpp11-migrate/UseAuto/basic_iterator_tests.cpp:23
@@ -22,3 +22,3 @@
 // RUN: cpp11-migrate -use-auto %t.cpp -- -DCONTAINER=array \
 // RUN:   -DUSE_BASE_CLASS_ITERATORS -I %S/Inputs
 // RUN: FileCheck -input-file=%t.cpp %s
----------------
Since the `-DUSE_INLINE_NAMESPACE` has been fixed I would fix this one as well `-DUSE_BASE_CLASS_ITERATORS=1` .

================
Comment at: test/cpp11-migrate/UseAuto/basic_iterator_tests.cpp:28
@@ -27,3 +27,3 @@
 // RUN: cpp11-migrate -use-auto %t.cpp -- -DCONTAINER=array \
 // RUN:   -DUSE_INNER_CLASS_ITERATORS -I %S/Inputs
 // RUN: FileCheck -input-file=%t.cpp %s
----------------
`-DUSE_INNER_CLASS_ITERATORS=1` as well here.

================
Comment at: test/cpp11-migrate/UseAuto/basic_iterator_tests.cpp:121
@@ -120,3 +120,3 @@
   }
 
   return 0;
----------------
I suggest to add this test here:
```
 {
    using std::CONTAINER;
    CONTAINER<int> C;
    CONTAINER<int>::iterator I = C.begin();
    // CHECK: auto I = C.begin();
  }
```

I may be wrong but I don't think these the tests in this file tested the inline namespace bug (when there is no nested name specifier with 'std').

================
Comment at: test/cpp11-migrate/UseAuto/Inputs/test_std_container.h:117
@@ -116,3 +116,3 @@
 } // namespace _1
 using _1::CONTAINER;
 #endif
----------------
This line should be removed. The declarations in an inline namespace are available in its 'outer scope'.

================
Comment at: cpp11-migrate/UseAuto/UseAutoMatchers.cpp:171
@@ +170,3 @@
+/// \c q.
+AST_MATCHER(NestedNameSpecifier, hasCanonicalTypeFromStd) {
+  if (const Type *T = Node.getAsType()) {
----------------
I thought the matcher from ReplaceAutoPtr that deals with std declarations could solve this issue as well, any reason not to use it?

For information here is the matcher:
```

/// \brief Matches declarations whose declaration context is the C++ standard
/// library namespace \c std.
///
/// Note that inline namespaces are silently ignored during the lookup since
/// both libstdc++ and libc++ are known to use them for versioning purposes.
///
/// Given
/// \code
///   namespace ns {
///     struct my_type {};
///     using namespace std;
///   }
///
///   using std::vector;
///   using ns::my_type;
///   using ns::list;
/// \endcode
/// usingDecl(hasAnyUsingShadowDecl(hasTargetDecl(isFromStdNamespace())))
///   matches "using std::vector" and "using ns::list".
AST_MATCHER(Decl, isFromStdNamespace) {
  const DeclContext *D = Node.getDeclContext();

  while (D->isInlineNamespace())
    D = D->getParent();

  if (!D->isNamespace() || !D->getParent()->isTranslationUnit())
    return false;

  const IdentifierInfo *Info = cast<NamespaceDecl>(D)->getIdentifier();

  return Info && Info->isStr("std");
}
```

Not tested but I think the matcher `typedefIterator()` could be rewritten for example that way:
```
// \brief Returns a TypeMatcher that matches typedefs for standard iterators
// inside records with a standard container name.
TypeMatcher typedefIterator() {
  return typedefType(
           hasDeclaration(
             allOf(
               namedDecl(hasStdIteratorName()),
               hasDeclContext(
-                 recordDecl(hasStdContainerName(true))
+                 recordDecl(hasStdContainerName(), isFromStdNamespace())
               )
             )
           )
         );
}
```

If for some reason it can't be used as-is nor improved I think the logic can be re-used by the matcher `hasStdContainerName()`, the  bool argument `WithStd` can be removed (and then the UseAuto matchers can be refactored a bit).


http://llvm-reviews.chandlerc.com/D1116



More information about the cfe-commits mailing list