This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by krystyna on Apr 9 2016, 2:21 AM.

Details

Diff Detail

Event Timeline

krystyna updated this revision to Diff 53112.Apr 9 2016, 2:21 AM
krystyna retitled this revision from to Add check "modernize use using" .
krystyna updated this object.
krystyna added reviewers: Prazek, mnbvmar, staronj, alexfh.
krystyna added a subscriber: cfe-commits.
staronj added inline comments.Apr 9 2016, 7:06 AM
clang-tidy/modernize/UseUsingCheck.h
20

Fix the FIXME.

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

Add end of line at the end of file.

Prazek added inline comments.Apr 9 2016, 7:10 AM
docs/clang-tidy/checks/modernize-use-using.rst
14

add cases with pointers to function / members

Eugene.Zelenko retitled this revision from Add check "modernize use using" to [Clang-tidy] Add check "modernize use using" .Apr 9 2016, 12:12 PM
Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

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

Prazek added inline comments.Apr 9 2016, 12:41 PM
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

Prazek requested changes to this revision.Apr 11 2016, 3:04 PM
Prazek edited edge metadata.
This revision now requires changes to proceed.Apr 11 2016, 3:04 PM
krystyna updated this revision to Diff 54007.Apr 17 2016, 9:30 AM
krystyna edited edge metadata.
krystyna removed rL LLVM as the repository for this revision.
krystyna marked 5 inline comments as done.
krystyna edited edge metadata.
krystyna set the repository for this revision to rL LLVM.
alexfh edited reviewers, added: hokein; removed: alexfh.Apr 21 2016, 8:00 AM
hokein added inline comments.Apr 24 2016, 11:34 AM
clang-tidy/modernize/ModernizeTidyModule.cpp
49

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

clang-tidy/modernize/UseUsingCheck.cpp
23

Add a blank line.

24

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

25

replace parameter can be const std::string&.

26

code indentation.

38

const auto &p

41

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

46

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

clang-tidy/modernize/UseUsingCheck.h
19

Please use a complete sentence.

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

Remove the extra blank line.

9

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

krystyna updated this revision to Diff 54974.Apr 26 2016, 2:25 AM
krystyna marked an inline comment as done.
krystyna marked 8 inline comments as done.May 2 2016, 12:17 PM
krystyna updated this revision to Diff 55867.May 2 2016, 12:39 PM

hide done comments

krystyna marked 4 inline comments as done.May 2 2016, 12:45 PM
Prazek added a comment.May 2 2016, 2:29 PM

lgtm, but I'd rather see Hokein acceptance.

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

hokein accepted this revision.May 3 2016, 12:53 AM
hokein edited edge metadata.

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 added inline comments.May 3 2016, 1:34 AM
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 added inline comments.May 3 2016, 2:48 AM
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 added inline comments.May 3 2016, 3:01 AM
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."

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?

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.

krystyna updated this revision to Diff 57007.May 12 2016, 2:13 AM
krystyna edited edge metadata.
krystyna marked 8 inline comments as done.
Prazek added inline comments.May 12 2016, 2:24 AM
clang-tidy/modernize/UseUsingCheck.cpp
14

Is this required?

Eugene.Zelenko added inline comments.May 12 2016, 8:41 AM
clang-tidy/modernize/UseUsingCheck.cpp
14

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

etienneb added inline comments.
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;

krystyna updated this revision to Diff 60697.Jun 14 2016, 9:54 AM
Prazek accepted this revision.Jun 14 2016, 3:24 PM
Prazek edited edge metadata.

Macro shipit:

LGTM

clang-tidy/modernize/UseUsingCheck.cpp
72

There should be method called 'isInvalid()'

This revision is now accepted and ready to land.Jun 14 2016, 3:24 PM
krystyna updated this revision to Diff 61895.Jun 25 2016, 10:42 AM
krystyna edited edge metadata.
krystyna marked 5 inline comments as done.
Prazek closed this revision.Jun 27 2016, 6:10 AM

Please see PR28334.

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