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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
