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=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 ) {
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(
-
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 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 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
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
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
[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()
