Re: [PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files
alexfh added a comment. And thank you for the fix, btw! Repository: rL LLVM http://reviews.llvm.org/D16953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files
alexfh added a comment. I've committed the changes to the check code. Can you submit the change to the script as a separate patch? Repository: rL LLVM http://reviews.llvm.org/D16953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files
This revision was automatically updated to reflect the committed changes. Closed by commit rL260948: [clang-tidy] Enhance modernize-redundant-void-arg check to apply fixes to… (authored by alexfh). Changed prior to commit: http://reviews.llvm.org/D16953?vs=47319&id=48054#toc Repository: rL LLVM http://reviews.llvm.org/D16953 Files: clang-tools-extra/trunk/clang-tidy/modernize/RedundantVoidArgCheck.cpp Index: clang-tools-extra/trunk/clang-tidy/modernize/RedundantVoidArgCheck.cpp === --- clang-tools-extra/trunk/clang-tidy/modernize/RedundantVoidArgCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/modernize/RedundantVoidArgCheck.cpp @@ -46,42 +46,30 @@ namespace modernize { void RedundantVoidArgCheck::registerMatchers(MatchFinder *Finder) { - Finder->addMatcher(functionDecl(isExpansionInMainFile(), parameterCountIs(0), - unless(isImplicit()), unless(isExternC())) + Finder->addMatcher(functionDecl(parameterCountIs(0), unless(isImplicit()), + unless(isExternC())) .bind(FunctionId), this); - Finder->addMatcher(typedefDecl(isExpansionInMainFile()).bind(TypedefId), - this); + Finder->addMatcher(typedefDecl().bind(TypedefId), this); auto ParenFunctionType = parenType(innerType(functionType())); auto PointerToFunctionType = pointee(ParenFunctionType); auto FunctionOrMemberPointer = anyOf(hasType(pointerType(PointerToFunctionType)), hasType(memberPointerType(PointerToFunctionType))); - Finder->addMatcher( - fieldDecl(isExpansionInMainFile(), FunctionOrMemberPointer).bind(FieldId), - this); - Finder->addMatcher( - varDecl(isExpansionInMainFile(), FunctionOrMemberPointer).bind(VarId), - this); + Finder->addMatcher(fieldDecl(FunctionOrMemberPointer).bind(FieldId), this); + Finder->addMatcher(varDecl(FunctionOrMemberPointer).bind(VarId), this); auto CastDestinationIsFunction = hasDestinationType(pointsTo(ParenFunctionType)); Finder->addMatcher( - cStyleCastExpr(isExpansionInMainFile(), CastDestinationIsFunction) - .bind(CStyleCastId), - this); + cStyleCastExpr(CastDestinationIsFunction).bind(CStyleCastId), this); Finder->addMatcher( - cxxStaticCastExpr(isExpansionInMainFile(), CastDestinationIsFunction) - .bind(NamedCastId), - this); + cxxStaticCastExpr(CastDestinationIsFunction).bind(NamedCastId), this); Finder->addMatcher( - cxxReinterpretCastExpr(isExpansionInMainFile(), CastDestinationIsFunction) - .bind(NamedCastId), + cxxReinterpretCastExpr(CastDestinationIsFunction).bind(NamedCastId), this); Finder->addMatcher( - cxxConstCastExpr(isExpansionInMainFile(), CastDestinationIsFunction) - .bind(NamedCastId), - this); - Finder->addMatcher(lambdaExpr(isExpansionInMainFile()).bind(LambdaId), this); + cxxConstCastExpr(CastDestinationIsFunction).bind(NamedCastId), this); + Finder->addMatcher(lambdaExpr().bind(LambdaId), this); } void RedundantVoidArgCheck::check(const MatchFinder::MatchResult &Result) { Index: clang-tools-extra/trunk/clang-tidy/modernize/RedundantVoidArgCheck.cpp === --- clang-tools-extra/trunk/clang-tidy/modernize/RedundantVoidArgCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/modernize/RedundantVoidArgCheck.cpp @@ -46,42 +46,30 @@ namespace modernize { void RedundantVoidArgCheck::registerMatchers(MatchFinder *Finder) { - Finder->addMatcher(functionDecl(isExpansionInMainFile(), parameterCountIs(0), - unless(isImplicit()), unless(isExternC())) + Finder->addMatcher(functionDecl(parameterCountIs(0), unless(isImplicit()), + unless(isExternC())) .bind(FunctionId), this); - Finder->addMatcher(typedefDecl(isExpansionInMainFile()).bind(TypedefId), - this); + Finder->addMatcher(typedefDecl().bind(TypedefId), this); auto ParenFunctionType = parenType(innerType(functionType())); auto PointerToFunctionType = pointee(ParenFunctionType); auto FunctionOrMemberPointer = anyOf(hasType(pointerType(PointerToFunctionType)), hasType(memberPointerType(PointerToFunctionType))); - Finder->addMatcher( - fieldDecl(isExpansionInMainFile(), FunctionOrMemberPointer).bind(FieldId), - this); - Finder->addMatcher( - varDecl(isExpansionInMainFile(), FunctionOrMemberPointer).bind(VarId), - this); + Finder->addMatcher(fieldDecl(FunctionOrMemberPointer).bind(FieldId), this); + Finder->addMatcher(varDecl(FunctionOrMemberPointer).bind(VarId), this); auto CastDestinationIsFunction = hasDestinationType(pointsTo(ParenFunctionType)); Finder->addMatcher( - cStyleCastExpr(isExpansionIn
Re: [PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files
alexfh added a comment. Unfortunately, the review of the changes in the script might take some time. I think, we can submit the fix itself already and work on the rest of the patch after that. http://reviews.llvm.org/D16953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files
LegalizeAdulthood added a comment. Squeak? http://reviews.llvm.org/D16953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files
LegalizeAdulthood updated this revision to Diff 47319. LegalizeAdulthood added a comment. Update from review comments http://reviews.llvm.org/D16953 Files: clang-tidy/modernize/RedundantVoidArgCheck.cpp test/clang-tidy/check_clang_tidy.py test/clang-tidy/modernize-redundant-void-arg.cpp test/clang-tidy/modernize-redundant-void-arg.h Index: test/clang-tidy/modernize-redundant-void-arg.h === --- /dev/null +++ test/clang-tidy/modernize-redundant-void-arg.h @@ -0,0 +1,8 @@ +#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H +#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H + +extern int foo(void); +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: redundant void argument list in function declaration [modernize-redundant-void-arg] +// CHECK-FIXES: {{^}}extern int foo();{{$}} + +#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H Index: test/clang-tidy/modernize-redundant-void-arg.cpp === --- test/clang-tidy/modernize-redundant-void-arg.cpp +++ test/clang-tidy/modernize-redundant-void-arg.cpp @@ -1,4 +1,6 @@ -// RUN: %check_clang_tidy %s modernize-redundant-void-arg %t +// RUN: %check_clang_tidy --header-filter=modernize-redundant-void-arg.h %s modernize-redundant-void-arg %t + +#include "modernize-redundant-void-arg.h" #define NULL 0 Index: test/clang-tidy/check_clang_tidy.py === --- test/clang-tidy/check_clang_tidy.py +++ test/clang-tidy/check_clang_tidy.py @@ -25,6 +25,7 @@ """ import argparse +import os import re import subprocess import sys @@ -35,16 +36,98 @@ f.write(text) f.truncate() + +# Remove the contents of the CHECK lines to avoid CHECKs matching on +# themselves. We need to keep the comments to preserve line numbers while +# avoiding empty lines which could potentially trigger formatting-related +# checks. +def clean_text(input_text): + return re.sub('// *CHECK-[A-Z-]*:[^\r\n]*', '//', input_text) + + +def write_cleaned_header(input_file_path, temp_file_name, header_file_name): + src_path = os.path.join(os.path.dirname(input_file_path), header_file_name) + fixed_path = os.path.join(os.path.dirname(temp_file_name), header_file_name) + with open(src_path, 'r') as input_file: +cleaned_text = clean_text(input_file.read()) + cleaned_path = fixed_path + '.orig' + write_file(cleaned_path, cleaned_text) + write_file(fixed_path, cleaned_text) + return cleaned_path, fixed_path, src_path + + +def separate_output(clang_tidy_output, header_file_name, input_file_name): + input_file_name = os.path.basename(input_file_name) + input_file_output = '' + header_file_output = '' + input_messages = True + for line in clang_tidy_output.splitlines(True): +if input_messages: + if header_file_name in line: +input_messages = False +header_file_output += line + else: +input_file_output += line +else: + if input_file_name in line: +input_messages = True +input_file_output += line + else: +header_file_output += line + return header_file_output, input_file_output + + +def try_run(args): + try: +process_output = \ + subprocess.check_output(args, stderr=subprocess.STDOUT).decode() + except subprocess.CalledProcessError as e: +print('%s failed:\n%s' % (' '.join(args), e.output.decode())) +raise + return process_output + + +def check_output_for_messages(messages_file, input_file_name): + try_run( +['FileCheck', '-input-file=' + messages_file, input_file_name, + '-check-prefix=CHECK-MESSAGES', + '-implicit-check-not={{warning|error}}:']) + + +def check_output_for_fixes(temp_file_name, input_file_name): + try_run( + ['FileCheck', '-input-file=' + temp_file_name, input_file_name, + '-check-prefix=CHECK-FIXES', '-strict-whitespace']) + + +def has_checks(input_text): + has_check_fixes = input_text.find('CHECK-FIXES') >= 0 + has_check_messages = input_text.find('CHECK-MESSAGES') >= 0 + return has_check_fixes, has_check_messages + + +def diff_source_files(original_file_name, temp_file_name): + try: +diff_output = subprocess.check_output( + ['diff', '-u', original_file_name, temp_file_name], + stderr=subprocess.STDOUT) + except subprocess.CalledProcessError as e: +diff_output = e.output + return diff_output + + def main(): parser = argparse.ArgumentParser() parser.add_argument('-resource-dir') + parser.add_argument('--header-filter') parser.add_argument('input_file_name') parser.add_argument('check_name') parser.add_argument('temp_file_name') args, extra_args = parser.parse_known_args() resource_dir = args.resource_dir + header_filter = args.header_filter input_file_name = args.input_file_name check_name = args.check_name temp_file_name
Re: [PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files
LegalizeAdulthood marked an inline comment as done. Comment at: test/clang-tidy/check_clang_tidy.py:152 @@ -68,4 +151,3 @@ - has_check_fixes = input_text.find('CHECK-FIXES') >= 0 - has_check_messages = input_text.find('CHECK-MESSAGES') >= 0 + has_check_fixes, has_check_messages = has_checks(input_text) LegalizeAdulthood wrote: > alexfh wrote: > > This function doesn't look like a particularly useful one: 5 lines of code > > instead of 2 lines and an unclear interface (in which order do the two > > bools come?). Please revert this part. > I initially had it doing the same checks on the header file as well and then > it made more sense. I dug myself into a dead-end on that series of changes, > reverted and started over. > > What I settled on here was the assumption that you won't have > `CHECK-MESSAGES` or `CHECK-FIXES` in your header file, unless you also had > them in the associated source file. If that assumption is invalid, then I > should call this function again for the header and change the tests to be > asserting fixes in the source or header, messages in the source or header in > various places. IMO, "5 lines instead of 2 lines" isn't the kind of reasoning that is useful. Otherwise we would never do [[ https://www.industriallogic.com/xp/refactoring/composeMethod.html | Compose Method ]] on a long method since the act of breaking a long method down into a bunch of smaller methods necessarily introduces more lines of code. This script originally was one giant function that did everything. A bunch of things need to be done twice now: one for the source file and once for the header file. Applying Compose Method Extracting results in extracting a group of smaller functions/methods. Now the functions have a bunch of state that needs to be passed around between them and the functions might be more clearly expressed as methods on a class, but I didn't go that far with it. http://reviews.llvm.org/D16953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files
LegalizeAdulthood added inline comments. Comment at: test/clang-tidy/check_clang_tidy.py:122 @@ -40,2 +121,3 @@ parser.add_argument('-resource-dir') + parser.add_argument('--header-filter') parser.add_argument('input_file_name') alexfh wrote: > There's no need for a separate argument for this. The script passes all > arguments after a `--` to clang-tidy, so you can just append `-- > -header-filter=... -- -std=c++11` to the RUN: line in the test. This argument is the key to making everything work. It triggers the copying of the header, the cleaning of the header of `CHECK-FIXES` and `CHECK-MESSAGES`, the diffing of the header, etc. I originally had it like you suggested by trying to have the test file pass all the necessary extra arguments to clang-tidy, but it just isn't enough. The script needs to do a bunch of extra work when headers are involved, so it seemed to make the most sense to pass the argument directly to the script so that it knows to do this extra work. In other words, express the intent directly to the script instead of having the script infer intent by scraping through extra arguments. Additionally, this preserves the behavior of eliminating duplicated arguments across all clang-tidy tests because the script will assume `-std=c++11` for `.cpp` files. Comment at: test/clang-tidy/check_clang_tidy.py:152 @@ -68,4 +151,3 @@ - has_check_fixes = input_text.find('CHECK-FIXES') >= 0 - has_check_messages = input_text.find('CHECK-MESSAGES') >= 0 + has_check_fixes, has_check_messages = has_checks(input_text) alexfh wrote: > This function doesn't look like a particularly useful one: 5 lines of code > instead of 2 lines and an unclear interface (in which order do the two bools > come?). Please revert this part. I initially had it doing the same checks on the header file as well and then it made more sense. I dug myself into a dead-end on that series of changes, reverted and started over. What I settled on here was the assumption that you won't have `CHECK-MESSAGES` or `CHECK-FIXES` in your header file, unless you also had them in the associated source file. If that assumption is invalid, then I should call this function again for the header and change the tests to be asserting fixes in the source or header, messages in the source or header in various places. http://reviews.llvm.org/D16953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files
alexfh added a comment. Hi Richard, Thank you for working on this. The script has been begging for refactoring for a while ;) A couple of initial comments, and while I'm looking further into into this patch, could you find other possible usages (in currently existing tests) of the new functionality in check_clang_tidy.py? Thanks! Comment at: test/clang-tidy/check_clang_tidy.py:82 @@ +81,3 @@ + try: +clang_tidy_output = \ + subprocess.check_output(args, stderr=subprocess.STDOUT).decode() Looks like this function is used not only for clang-tidy. Maybe this variable should be named more generically? Comment at: test/clang-tidy/check_clang_tidy.py:122 @@ -40,2 +121,3 @@ parser.add_argument('-resource-dir') + parser.add_argument('--header-filter') parser.add_argument('input_file_name') There's no need for a separate argument for this. The script passes all arguments after a `--` to clang-tidy, so you can just append `-- -header-filter=... -- -std=c++11` to the RUN: line in the test. Comment at: test/clang-tidy/check_clang_tidy.py:152 @@ -68,4 +151,3 @@ - has_check_fixes = input_text.find('CHECK-FIXES') >= 0 - has_check_messages = input_text.find('CHECK-MESSAGES') >= 0 + has_check_fixes, has_check_messages = has_checks(input_text) This function doesn't look like a particularly useful one: 5 lines of code instead of 2 lines and an unclear interface (in which order do the two bools come?). Please revert this part. http://reviews.llvm.org/D16953 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files
LegalizeAdulthood created this revision. LegalizeAdulthood added a reviewer: alexfh. LegalizeAdulthood added a subscriber: cfe-commits. Update `check_clang_tidy.py` to handle fixes applied to header files by adding `--header-filter` argument that: - Passes `-header-filter` down to `clang-tidy` - Copies named header to temporary directory where `clang-tidy` is run - Performs `FileCheck` checks on the header for `CHECK-MESSAGES` - Performs `FileCheck` checks on the header for `CHECK-FIXES` Fixes PR25894 http://reviews.llvm.org/D16953 Files: clang-tidy/modernize/RedundantVoidArgCheck.cpp test/clang-tidy/check_clang_tidy.py test/clang-tidy/modernize-redundant-void-arg.cpp test/clang-tidy/modernize-redundant-void-arg.h Index: test/clang-tidy/modernize-redundant-void-arg.h === --- /dev/null +++ test/clang-tidy/modernize-redundant-void-arg.h @@ -0,0 +1,8 @@ +#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H +#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H + +extern int foo(void); +// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: redundant void argument list in function declaration [modernize-redundant-void-arg] +// CHECK-FIXES: {{^}}extern int foo();{{$}} + +#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_MODERNIZE_REDUNDANT_VOID_ARG_H Index: test/clang-tidy/modernize-redundant-void-arg.cpp === --- test/clang-tidy/modernize-redundant-void-arg.cpp +++ test/clang-tidy/modernize-redundant-void-arg.cpp @@ -1,4 +1,6 @@ -// RUN: %check_clang_tidy %s modernize-redundant-void-arg %t +// RUN: %check_clang_tidy --header-filter=modernize-redundant-void-arg.h %s modernize-redundant-void-arg %t + +#include "modernize-redundant-void-arg.h" #define NULL 0 Index: test/clang-tidy/check_clang_tidy.py === --- test/clang-tidy/check_clang_tidy.py +++ test/clang-tidy/check_clang_tidy.py @@ -25,6 +25,7 @@ """ import argparse +import os import re import subprocess import sys @@ -35,16 +36,98 @@ f.write(text) f.truncate() + +# Remove the contents of the CHECK lines to avoid CHECKs matching on +# themselves. We need to keep the comments to preserve line numbers while +# avoiding empty lines which could potentially trigger formatting-related +# checks. +def clean_text(input_text): + return re.sub('// *CHECK-[A-Z-]*:[^\r\n]*', '//', input_text) + + +def write_cleaned_header(input_file_path, temp_file_name, header_file_name): + src_path = os.path.join(os.path.dirname(input_file_path), header_file_name) + fixed_path = os.path.join(os.path.dirname(temp_file_name), header_file_name) + with open(src_path, 'r') as input_file: +cleaned_text = clean_text(input_file.read()) + cleaned_path = fixed_path + '.orig' + write_file(cleaned_path, cleaned_text) + write_file(fixed_path, cleaned_text) + return cleaned_path, fixed_path, src_path + + +def separate_output(clang_tidy_output, header_file_name, input_file_name): + input_file_name = os.path.basename(input_file_name) + input_file_output = '' + header_file_output = '' + input_messages = True + for line in clang_tidy_output.splitlines(True): +if input_messages: + if header_file_name in line: +input_messages = False +header_file_output += line + else: +input_file_output += line +else: + if input_file_name in line: +input_messages = True +input_file_output += line + else: +header_file_output += line + return header_file_output, input_file_output + + +def try_run(args): + try: +clang_tidy_output = \ + subprocess.check_output(args, stderr=subprocess.STDOUT).decode() + except subprocess.CalledProcessError as e: +print('%s failed:\n%s' % (' '.join(args), e.output.decode())) +raise + return clang_tidy_output + + +def check_output_for_messages(messages_file, input_file_name): + try_run( +['FileCheck', '-input-file=' + messages_file, input_file_name, + '-check-prefix=CHECK-MESSAGES', + '-implicit-check-not={{warning|error}}:']) + + +def check_output_for_fixes(temp_file_name, input_file_name): + try_run( + ['FileCheck', '-input-file=' + temp_file_name, input_file_name, + '-check-prefix=CHECK-FIXES', '-strict-whitespace']) + + +def has_checks(input_text): + has_check_fixes = input_text.find('CHECK-FIXES') >= 0 + has_check_messages = input_text.find('CHECK-MESSAGES') >= 0 + return has_check_fixes, has_check_messages + + +def diff_source_files(original_file_name, temp_file_name): + try: +diff_output = subprocess.check_output( + ['diff', '-u', original_file_name, temp_file_name], + stderr=subprocess.STDOUT) + except subprocess.CalledProcessError as e: +diff_output = e.output + return diff_output + + def main(): parser = argparse.ArgumentParser() parser.add_argument('