[PATCH] D73090: [clang-tidy] Fix PR#44528 'modernize-use-using and enums'

[Karasev Nikita via Phabricator via cfe-commits]

f00kat marked 4 inline comments as done.
f00kat added a comment.

I have changed the code to work with tagDecl. It`s work fine on simple test cases such as

  typedef enum { ea, eb } EnumT;

But not work with

  #include <string>
  typedef enum { ea, eb } EnumT;

It is not related with new tagDecl matcher. I simplify this case:

1. Create file Header.h
2. Header.h

  typedef size_t sometype;

3. main.cpp

  #include "Header.h"
  using EnumT = enum { ea, eb };

It`s happens because in class UseUsingCheck we have variable LastReplacementEnd and it may contains SourceLocation for another file.
First time AST matcher trigger on file Header.h and we remember SourceLocation for code "typedef size_t sometype". 
Then AST matcher trigger on "using EnumT = enum { ea, eb };" in main.cpp and we trying to use the value of LastReplacementEnd storing SourceLocation for Header.h.

So this 'if case' does not execute

  if (ReplaceRange.getBegin().isMacroID() ||
     ReplaceRange.getBegin() >= LastReplacementEnd) {

execute this one

  // This is additional TypedefDecl in a comma-separated typedef declaration.
  // Start replacement *after* prior replacement and separate with semicolon.
  ReplaceRange.setBegin(LastReplacementEnd);

Simple fix

  if (ReplaceRange.getBegin().isMacroID() ||
     (Result.SourceManager->getFileID(ReplaceRange.getBegin()) != Result.SourceManager->getFileID(LastReplacementEnd)) || \\ new check
     (ReplaceRange.getBegin() >= LastReplacementEnd)) {

But maybe I wrong. I don`t know API well enough.



================
Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:28
                      this);
-  // This matcher used to find structs defined in source code within typedefs.
+  // This matcher`s used to find structs/enums defined in source code within typedefs.
   // They appear in the AST just *prior* to the typedefs.
----------------
aaron.ballman wrote:
> It looks like there's a backtick in the comment rather than a quotation mark, that should probably be `matcher's` instead. Also, instead of `struct/enums`, I think it should be `tag declarations` (unions count, for instance).
Ok


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:30-31
   // They appear in the AST just *prior* to the typedefs.
-  Finder->addMatcher(cxxRecordDecl(unless(isImplicit())).bind("struct"), this);
+  Finder->addMatcher(cxxRecordDecl(unless(isImplicit())).bind("tagdecl"), this);
+  Finder->addMatcher(enumDecl(unless(isImplicit())).bind("tagdecl"), this);
 }
----------------
aaron.ballman wrote:
> Rather than matching on these, I think it would make more sense to add a new matcher for `tagDecl()`. It can be local to the check for now, or you can add it to the AST matchers header as a separate patch and then base this patch off that work.
"Add new AST Matcher" - it's the Jedi level :)
Where can I find some manual that describe this? Or maybe some example(git commit).

"you can add it to the AST matchers header as a separate patch and then base this patch off that work."
So the sequencing will be:
1. I create another new patch with new AST matcher 'tagDecl'
2. Wait until you reviewed it
3. Merge to master
4. Pull latest master
4. Modify code and update this patch
?


================
Comment at: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp:30-31
   // They appear in the AST just *prior* to the typedefs.
-  Finder->addMatcher(cxxRecordDecl(unless(isImplicit())).bind("struct"), this);
+  Finder->addMatcher(cxxRecordDecl(unless(isImplicit())).bind("tagdecl"), this);
+  Finder->addMatcher(enumDecl(unless(isImplicit())).bind("tagdecl"), this);
 }
----------------
f00kat wrote:
> aaron.ballman wrote:
> > Rather than matching on these, I think it would make more sense to add a new matcher for `tagDecl()`. It can be local to the check for now, or you can add it to the AST matchers header as a separate patch and then base this patch off that work.
> "Add new AST Matcher" - it's the Jedi level :)
> Where can I find some manual that describe this? Or maybe some example(git commit).
> 
> "you can add it to the AST matchers header as a separate patch and then base this patch off that work."
> So the sequencing will be:
> 1. I create another new patch with new AST matcher 'tagDecl'
> 2. Wait until you reviewed it
> 3. Merge to master
> 4. Pull latest master
> 4. Modify code and update this patch
> ?
I add new TagDecl matcher in this patch https://reviews.llvm.org/D73464. Check please.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp:274
+// CHECK-FIXES: using EnumT = enum { ea, eb };
\ No newline at end of file

----------------
njames93 wrote:
> Can you put a new line here.
Yeah


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73090/new/

https://reviews.llvm.org/D73090