[Clang-tidy] Add check "modernize use using"

[staronj]

clang-tidy/modernize/UseUsingCheck.h

20

Fix the FIXME.

test/clang-tidy/modernize-use-using.cpp

88

Add end of line at the end of file.

[Prazek]

docs/clang-tidy/checks/modernize-use-using.rst

14

add cases with pointers to function / members

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

[Prazek]

clang-tidy/modernize/UseUsingCheck.cpp

27

add static

28

Stick to the LLVM coding style - local variables starts with capital letter.
Also add const here

33–36

you can extract this to another static function
static void replaceAll(std::string &s, const std::string &search, const std::string replace);

49

check if it is clang-formatted

[hokein]

clang-tidy/modernize/ModernizeTidyModule.cpp

49

You can put it in one-line, which will not exceed 80 characters.

clang-tidy/modernize/UseUsingCheck.cpp

24

Add a blank line.

25

A blank between /// and F, please use clang-format to make sure your code align with LLVM style.

26

replace parameter can be const std::string&.

27

code indentation.

39

const auto &p

42

You can use replaceAll(subject, p.first, p.second).

47

You need to filter out the non-c++11 code here.

clang-tidy/modernize/UseUsingCheck.h

20

Please use a complete sentence.

test/clang-tidy/modernize-use-using.cpp

3

Remove the extra blank line.

10

No need to check the whole message except the first one. So here you can use warning: use using instead of typedef.

[krystyna]

hide done comments

[Prazek]

lgtm, but I'd rather see Hokein acceptance.

[Eugene.Zelenko]

Check is still not mentioned in docs/ReleaseNotes.rst.

[hokein]

LGTM with some nits.

clang-tidy/modernize/UseUsingCheck.cpp

23

Should be CplusPlus11 here.

test/clang-tidy/modernize-use-using.cpp

29

code indentation.

30

The same.

45

The same.

[Prazek]

clang-tidy/modernize/UseUsingCheck.cpp

23

BTW is there any policy about that? I see that some checks from modernize require C++11 (e.g. modernize-make-unique which is in C++14) and other require just C++ (modernize-loop-convert), and it even has a comment

// Only register the matchers for C++. Because this checker is used for

// modernization, it is reasonable to run it on any C++ standard with the
// assumption the user is trying to modernize their codebase.
if (!getLangOpts().CPlusPlus)
  return;

I have 2 thoughts for this:

1. there should be note in documentation about it, so the user won't spend time debuging why the check doesn't do anything. e.g. "This check requires to compile code with C++11 or higher"
2. I would suggest modernize checks to require standard version the same or higher for C++ standars that doesn't break backwards compatibility:

e.g. loop-convert should require C++11, make-shared C++14, this check also C++11,
but for modernize-increment-bool, that is deprecated in C++17, it should require just C++ (because if someone need it, the he wont be able to compile his code with C++17).

[hokein]

clang-tidy/modernize/UseUsingCheck.cpp

23

This is a good point.

As far as I know, we don't have detailed policy about the modernized checks. It depends on the check author. Basically the modern words means "C++11" feature.

I'm +1 on adding a note in each modernized check's document.

[Prazek]

clang-tidy/modernize/UseUsingCheck.cpp

23

Cool. I sent email to mailing list to ask other devs for opinion.

Krystyna, please add note at the end of documentation

"This check requires using C++11 or higher to run."

[curdeius]

I'm really interested in the manner this check works when a typedef has multiple declarations in it (same example as in the comment):

typedef int m_int, *m_int_p, &m_int_r, m_int_arr[10], (&m_int_fun)(int, int);

I tried to implement such a check once, but this was the hard part. FYI, that's my stub: https://github.com/llvm-mirror/clang-tools-extra/compare/master...mkurdej:feature/use-using.

docs/clang-tidy/checks/modernize-use-using.rst

22

Typo: varible -> variable.

test/clang-tidy/modernize-use-using.cpp

2

Could you add tests for the case where there are multiple type declarations in one statement? E.g.:

typedef int m_int, *m_int_p, &m_int_r, m_int_arr[10], (&m_int_fun)(int, int);

Would it be transformed to multiple using statements or only a warning will be emitted?

[Prazek]

In D18919#419902, @curdeius wrote:

I'm really interested in the manner this check works when a typedef has multiple declarations in it (same example as in the comment):

typedef int m_int, *m_int_p, &m_int_r, m_int_arr[10], (&m_int_fun)(int, int);

I tried to implement such a check once, but this was the hard part. FYI, that's my stub: https://github.com/llvm-mirror/clang-tools-extra/compare/master...mkurdej:feature/use-using.

I don't think that this featcher is necessary. I don't know a lot of code using it. I am curious how this check will behave.

[Prazek]

clang-tidy/modernize/UseUsingCheck.cpp

14

Is this required?

[Eugene.Zelenko]

clang-tidy/modernize/UseUsingCheck.cpp

14

It'll be reasonable to run Include What You Use at least on new files.

[etienneb]

clang-tidy/modernize/UseUsingCheck.cpp

33

I think 'using' should be quote.
This message is hard to read and understand from a user point of view.

test/clang-tidy/modernize-use-using.cpp

49

Could you add test with macro:

#define CODE \

typedef int INT; \
[....]

CODE;

[Prazek]

LGTM

clang-tidy/modernize/UseUsingCheck.cpp

72

There should be method called 'isInvalid()'

[Eugene.Zelenko]

Please see PR28334.

I'm not sure if Krystyna has Bugzilla account, so I report problem here.

clang-tidy/modernize/CMakeLists.txt

   UseEmplaceCheck.cpp
   UseNullptrCheck.cpp
   UseOverrideCheck.cpp
+  UseUsingCheck.cpp
 
   LINK_LIBS
   clangAST

clang-tidy/modernize/ModernizeTidyModule.cpp

 #include "UseEmplaceCheck.h"
 #include "UseNullptrCheck.h"
 #include "UseOverrideCheck.h"
+#include "UseUsingCheck.h"
 
 using namespace clang::ast_matchers;
 

     CheckFactories.registerCheck<UseEmplaceCheck>("modernize-use-emplace");
     CheckFactories.registerCheck<UseNullptrCheck>("modernize-use-nullptr");
     CheckFactories.registerCheck<UseOverrideCheck>("modernize-use-override");
+    CheckFactories.registerCheck<UseUsingCheck>("modernize-use-using");
   }
 
   ClangTidyOptions getModuleOptions() override {

clang-tidy/modernize/UseUsingCheck.h

+//===--- UseUsingCheck.h - clang-tidy----------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_USING_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_USING_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// Check finds typedefs and replaces it with usings.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-using.html
+class UseUsingCheck : public ClangTidyCheck {
+public:
+  UseUsingCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_USING_H

clang-tidy/modernize/UseUsingCheck.cpp

+//===--- UseUsingCheck.cpp - clang-tidy------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "UseUsingCheck.h"
+#include "../utils/LexerUtils.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+void UseUsingCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus11)
+    return;
+  Finder->addMatcher(typedefDecl().bind("typedef"), this);
+}
+
+// Checks if 'typedef' keyword can be removed - we do it only if
+// it is the only declaration in a declaration chain.
+static bool CheckRemoval(SourceManager &SM, const SourceLocation &LocStart,
+                         const SourceLocation &LocEnd, ASTContext &Context,
+                         SourceRange &ResultRange) {
+  FileID FID = SM.getFileID(LocEnd);
+  llvm::MemoryBuffer *Buffer = SM.getBuffer(FID, LocEnd);
+  Lexer DeclLexer(SM.getLocForStartOfFile(FID), Context.getLangOpts(),
+                  Buffer->getBufferStart(), SM.getCharacterData(LocStart),
+                  Buffer->getBufferEnd());
+  Token DeclToken;
+  bool result = false;
+  int parenthesisLevel = 0;
+
+  while (!DeclLexer.LexFromRawLexer(DeclToken)) {
+    if (DeclToken.getKind() == tok::TokenKind::l_paren)
+      parenthesisLevel++;
+    if (DeclToken.getKind() == tok::TokenKind::r_paren)
+      parenthesisLevel--;
+    if (DeclToken.getKind() == tok::TokenKind::semi)
+      break;
+    // if there is comma and we are not between open parenthesis then it is
+    // two or more declatarions in this chain
+    if (parenthesisLevel == 0 && DeclToken.getKind() == tok::TokenKind::comma)
+      return false;
+
+    if (DeclToken.isOneOf(tok::TokenKind::identifier,
+                          tok::TokenKind::raw_identifier)) {
+      auto TokenStr = DeclToken.getRawIdentifier().str();
+
+      if (TokenStr == "typedef") {
+        ResultRange =
+            SourceRange(DeclToken.getLocation(), DeclToken.getEndLoc());
+        result = true;
+      }
+    }
+  }
+  // assert if there was keyword 'typedef' in declaration
+  assert(result && "No typedef found");
+
+  return result;
+}
+
+void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<TypedefDecl>("typedef");
+  if (MatchedDecl->getLocation().isInvalid())
+    return;
+
+  auto &Context = *Result.Context;
+  auto &SM = *Result.SourceManager;
+
+  auto Diag =
+      diag(MatchedDecl->getLocStart(), "use 'using' instead of 'typedef'");
+  if (MatchedDecl->getLocStart().isMacroID()) {
+    return;
+  }
+  SourceRange RemovalRange;
+  if (CheckRemoval(SM, MatchedDecl->getLocStart(), MatchedDecl->getLocEnd(),
+                   Context, RemovalRange)) {
+    Diag << FixItHint::CreateReplacement(
+        MatchedDecl->getSourceRange(),
+        "using " + MatchedDecl->getNameAsString() + " = " +
+            MatchedDecl->getUnderlyingType().getAsString(getLangOpts()));
+  }
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 
   Finds calls that could be changed to emplace.
 
+- New `modernize-use-using
+  <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-using.html>`_ check
+
+  Finds typedefs and replaces it with usings.
+
 - New `performance-faster-string-find
   <http://clang.llvm.org/extra/clang-tidy/checks/performance-faster-string-find.html>`_ check
 

docs/clang-tidy/checks/list.rst

    modernize-use-emplace
    modernize-use-nullptr
    modernize-use-override
+   modernize-use-using
    performance-faster-string-find
    performance-for-range-copy
    performance-implicit-cast-in-loop

docs/clang-tidy/checks/modernize-use-using.rst

+.. title:: clang-tidy - modernize-use-using
+
+modernize-use-using
+===================
+
+Use C++11's ``using`` instead of ``typedef``.
+
+Before:
+
+.. code:: c++
+
+  typedef int variable;
+
+  class Class{};
+  typedef void (Class::* MyPtrType)() const;
+
+After:
+
+.. code:: c++
+
+  using variable = int;
+
+  class Class{};
+  using MyPtrType = void (Class::*)() const;
+
+This check requires using C++11 or higher to run.

test/clang-tidy/modernize-use-using.cpp

+// RUN: %check_clang_tidy %s modernize-use-using %t
+
+typedef int Type;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef' [modernize-use-using]
+// CHECK-FIXES: using Type = int;
+
+typedef long LL;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using LL = long;
+
+typedef int Bla;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using Bla = int;
+
+typedef Bla Bla2;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using Bla2 = Bla;
+
+typedef void (*type)(int, int);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using type = void (*)(int, int);
+
+typedef void (*type2)();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using type2 = void (*)();
+
+class Class {
+  typedef long long Type;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef'
+  // CHECK-FIXES: using Type = long long;
+};
+
+typedef void (Class::*MyPtrType)(Bla) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using MyPtrType = void (Class::*)(Bla) const;
+
+class Iterable {
+public:
+  class Iterator {};
+};
+
+template <typename T>
+class Test {
+  typedef typename T::iterator Iter;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef'
+  // CHECK-FIXES: using Iter = typename T::iterator;
+};
+
+using balba = long long;
+
+union A {};
+
+typedef void (A::*PtrType)(int, int) const;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using PtrType = void (A::*)(int, int) const;
+
+typedef Class some_class;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using some_class = Class;
+
+typedef Class Cclass;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using Cclass = Class;
+
+typedef Cclass cclass2;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using cclass2 = Cclass;
+
+class cclass {};
+
+typedef void (cclass::*MyPtrType3)(Bla);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using MyPtrType3 = void (cclass::*)(Bla);
+
+using my_class = int;
+
+typedef Test<my_class *> another;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using another = Test<my_class *>;
+
+typedef int bla1, bla2, bla3;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+
+#define CODE typedef int INT
+
+CODE;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'