[PATCH] D16953: Enhance clang-tidy modernize-redundant-void-arg check to apply fixes to header files

Richard via cfe-commits cfe-commits at lists.llvm.org
Mon Feb 8 08:58:25 PST 2016


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





More information about the cfe-commits mailing list