This is an archive of the discontinued LLVM Phabricator instance.

clang-tidy: modernize-use-using work with multi-argument templates
ClosedPublic

Authored by poelmanc on Sep 11 2019, 12:45 PM.

Details

Summary

clang-tidy's modernize-use-using feature is great! But if it finds any commas that are not within parentheses, it won't create a fix. That means it won't change lines like:
typedef std::pair<int, int> Point;
to
using Point = std::pair<int, int>;
or even:
typedef std::map<std::string, Foo> MyMap;
typedef std::vector<int,MyCustomAllocator<int>> MyVector;

This patch allows the fix to apply to lines with commas if they are within parentheses or angle brackets that were not themselves within parentheses. Three tests included, latter due to jonathanmeier's reply.

(Also one additional line auto Diag = diag... was accidentally clang-formatted by my editor but it seemed like a clear improvement so I left it in.)

Diff Detail

Event Timeline

poelmanc created this revision.Sep 11 2019, 12:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 11 2019, 12:45 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
poelmanc edited the summary of this revision. (Show Details)Sep 11 2019, 12:50 PM

With non-type template parameters we can have expressions inside template arguments, including comparison operators like <, <=, > and >=, which lets us write typedefs like this:

template <bool B>
struct S {};

typedef S<(0 < 0)> S_t, *S_p;

Unfortunately, for this example your patch breaks the check in the tok::comma case, which should abort the removal when there are multiple declarations in the declaration chain. It thinks the comma is still part of the template argument, since it expects a matching end template angle bracket for the less than operator which was erroneously interpreted as the start of a template argument.

With the -fix option, clang-tidy produces the following invalid using declaration for the example above:

using S_t = S<(0 < 0)>, *S_p;
poelmanc updated this revision to Diff 219834.Sep 11 2019, 5:14 PM
poelmanc edited the summary of this revision. (Show Details)

Wow, thanks for the super-quick testing, feedback and a new test case! I added a slightly enhanced version of your test case to the modernize-use-using.cpp test file and got it passing by treating tok::less and tok::right as AngleBrackets only when ParenLevel == 0. See updated patch above. Holler if you think of any other stressing cases!

P.S. What do we think of actually handling the comma cases? (A) that's probably hard, and (B) as a C++ programmer I don't use them myself, so would we expand:

typedef S<(0 < 0)> S_t, *S_p;

to:

using S_t = S<(0 < 0)>;
using S_p = S_t*;

?

jonathanmeier added a comment.EditedSep 12 2019, 5:08 AM

Unfortunately, only considering parenthesis (l_paren, r_paren) is not sufficient. We need to consider all the nesting tokens, including brackets (l_square, r_square) and braces (l_brace, r_brace), since the Standard says (C++ 17 Draft N4659, Section 17.2/3):

[...] When parsing a template-argument-list, the first non-nested > is taken as the ending delimiter rather than a greater-than operator. [...]

Example with brackets that fails with your updated patch:

template <bool B>
struct S {};

constexpr bool b[1] = { true };

typedef S<b[0 < 0]> S_t, *S_p;

The following is an example with braces inside a template argument:

template <bool B>
struct S {};

struct T {
  constexpr T(bool) {}
  
  static constexpr bool b = true;  
};

typedef S<T{ 0 < 0 }.b> S_t, *S_p;

Note though, that the current code aborts upon encountering braces to avoid removing a typedef struct {...} T; case and therefore, isn't even able to handle a single declaration chain with braces in a template argument, such as typedef S<T{ 0 < 0 }.b> S_t;.

As to handling the comma cases:
(A) It is certainly not straightforward, since typedef declaration chains with multiple declarations are internally split up into separate declarations and the checker is called for each of these declarations individually.
(B) Your expansion would be valid, however, I think it might be easier to implement the expansion to

using S_t = S<(0 < 0)>;
using S_p = S<(0 < 0)>*;

Also note that your enhanced version of my first example actually works with your initial patch, since the two comparison operators cancel each other out in the counting logic.

poelmanc updated this revision to Diff 219957.Sep 12 2019, 11:38 AM

Thanks for the stressing test cases. I reverted to your original test case with a single >, added a separate one with a single <, and expanded the final case to have a non-balanced number of > and <. I added all your new cases, with variations for non-fixed (multiple typedef) and fixed (single typedef) examples.

To make the braces example pass, we now only abandon the fix when encountering a non-nested open brace.

Thanks, this looks much better now. I think there's no need to track nesting of parenthesis, brackets and braces separately, so we can collapse ParenLevel, BraceLevel and SquareBracketLevel into a single NestingLevel, which simplifies all the conditions quite a bit.

Also consider adding a few more testcases, especially with nested templates and multiple template arguments (the reason for this patch, after all! ;-) ). You can easily test (different numbers of) multiple template arguments by just adding a few more template parameters with default arguments to e.g. TwoArgTemplate, S or Q.

clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
71

change to If there is a non-nested comma we have two or more declarations in this chain.

poelmanc updated this revision to Diff 220002.Sep 12 2019, 2:36 PM

Nice catch, that simplifies the code quite a bit! I added two more nested, complex multiple-template-argument tests and am happy to add more.

(We could do a case fallthrough after tok::l_brace, though some linters warn about them.)

poelmanc updated this revision to Diff 220007.EditedSep 12 2019, 2:44 PM

Sorry added one more test at the end to make sure a multi-typedef with all that Variadic stuff still doesn't get changed to using.

Nice! I don't have commit access, so we'll need someone else have another look. @alexfh, since you previously worked on this, would you mind taking a look at this patch?

Hi @alexfh, @jonathanmeier has reviewed my pull request but lacks commit access. It changes ~30 lines of code isolated to modernize-use-using.cpp and adds ~60 lines of tests. If you have time I'd greatly appreciate it if you could provide any feedback or commit it. Alternatively can you suggest someone else who can review it? Thanks!

poelmanc updated this revision to Diff 225896.Oct 21 2019, 8:47 AM

Rebase to latest master (tests moved into new "checkers" subdirectory.)

poelmanc marked an inline comment as done.Oct 21 2019, 8:51 AM

Checked "Done". (I addressed @jonathanmeier's comment feedback with a previous update but forgot to check the box!)

I welcome any more feedback. Thanks.

aaron.ballman accepted this revision.Nov 7 2019, 12:21 PM

I'm a bit worried that this manual parsing technique will need fixing again in the future, but I think this is at least a reasonable incremental improvement.

LGTM with a minor nit.

clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
56

Might as well use ++NestingLevel given that you don't care about the result anyway. Similar below.

clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
197 ↗(On Diff #225896)

I was going to suggest another test involving attributes (which can have arbitrary expressions as arguments to the attribute), but then I discovered attributes in Clang are broken in the place where it would be an issue for this check anyway. :-D (I filed https://bugs.llvm.org/show_bug.cgi?id=43939 to address the attribute issue.)

This revision is now accepted and ready to land.Nov 7 2019, 12:21 PM

Thanks @aaron.ballman, I don't have commit access so will someone else commit this?

To address the minor nit, should I upload a new patch with post-increment/post-decrement changed to pre-increment/pre-decrement? (Does uploading changes undo the "Ready to Land" status?)

mgehre removed a subscriber: mgehre.Nov 9 2019, 12:27 AM

Thanks @aaron.ballman, I don't have commit access so will someone else commit this?

I can commit it for you when I get back into the office mid-next week, unless someone else wants to commit it on your behalf first.

To address the minor nit, should I upload a new patch with post-increment/post-decrement changed to pre-increment/pre-decrement? (Does uploading changes undo the "Ready to Land" status?)

Yes, please upload a new patch. It may change the review state in Phab, but unless it's a substantive change (which this is not), we do not require additional review/acceptance before landing (unless someone clicks the "Request Changes" option in the meantime).

poelmanc updated this revision to Diff 228767.Nov 11 2019, 1:44 PM

Changed post-increment/decrement to pre-increment/decrement.

poelmanc marked 2 inline comments as done.Nov 11 2019, 1:45 PM

Done, thanks.

poelmanc marked an inline comment as not done.Nov 14 2019, 1:22 PM

I'm a bit worried that this manual parsing technique will need fixing again in the future, but I think this is at least a reasonable incremental improvement.

Thanks and I agree. Your comments encouraged me to take another stab at improving things. See D70270, a whole new approach that removes manual parsing in favor of AST processing and successfully converts many more cases from typedef to using.

I didn't want to clobber this patch with that one in case any fatal flaws are discovered with the new approach. We can commit this one first - getting improved handling of commas and getting additional test cases - and treat D70270 as that next incremental improvement. Or we can keep this on hold until D70270 is worked out.

Thanks again for all your feedback and help!

poelmanc marked an inline comment as done.Nov 14 2019, 9:11 PM
This revision was automatically updated to reflect the committed changes.