[PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood abandoned this revision.
LegalizeAdulthood added a comment.
Herald added a subscriber: xazax.hun.
Herald added a reviewer: serge-sans-paille.

Review process takes too long to make forward progress; abandoning.


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

https://reviews.llvm.org/D17482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Let's try to turn this to a productive lane. I'm not against adding this 
functionality, if it's actually useful, but as is the patch doesn't meet the 
bar. Right now, there are a few action items.

First, could you split the refactorings and send me as a separate patch? They 
have little to do with the added functionality, so I prefer to review them 
separately.

Second, please move the header to a subdirectory of test/clang-tidy/Inputs/ 
(http://reviews.llvm.org/D17482#inline-156627).

Third (possibly, after the refactoring patch goes in, to reduce unneeded work), 
please address this comment: http://reviews.llvm.org/D17482#inline-149906.


http://reviews.llvm.org/D17482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D17482#390237, @alexfh wrote:

> My main concern is that this change makes the already complicated and 
> unobvious testing mechanism [...]


The complexity and obtuseness of the existing testing mechanism is unrelated to 
this changeset.  This changeset doesn't fundamentally change the testing 
mechanism.

> even more complicated and even less obvious for a very little (as I see it 
> now) benefit.


The benefit is that you would now have a consistent mechanism for verifying 
changes made to headers.

> The added functionality supports a very limited use case (a single header)


You have to start somewhere.  Currently there is no mechanism provided at all.  
Saying that this mechanism is not acceptable because it doesn't handle 
arbitrarily complex generalized checking is making the perfect the enemy of the 
good and isn't a reason for preventing this change from being accepted IMO.

> and it does it in a rather hacky way (the use of the new --header-filter flag 
> is not self-documenting and the overall behavior seems "magical" at the first 
> glance).


None of the mechanisms in the testing of these files is self-documenting.  If 
that is the criteria by which all changes are to be measured, then the entire 
existing system has to be thrown out.  Again, this feels like making the 
perfect the enemy of the good.  The behavior for validating header files is the 
same as the existing behavior for validating source files.  They are copied, 
filtered, processed and the processed results are analyzed.  Discrepencies are 
shown as diffs against the original source files.

> There's also an outstanding comment that you didn't seem to have addressed 
> yet.


Which specific comment are you referring to?  Because, just like this comment, 
there's a bunch of discussion in very general terms without specific complaints 
against specific pieces of code that are preventing it from being committed.

> In http://reviews.llvm.org/D17482#378685, @LegalizeAdulthood wrote:

> 

> > In the context of a clang-tidy check, what would be gained by verifying 
> > changes made to multiple headers that isn't already tested by verifying 
> > changes made to a single header?

> 


You haven't answered this question yet.  The existing test mechanism verifies 
only a single source file at a time; I see no reason why verifying a single 
header is such a great imposition of constraints that would prevent existing 
checks from being enhanced to verify their changes on header files.  However, 
should that arise in the future it is a relatively small change to add 
additional header processing to the script.  Again, let's not make the perfect 
the enemy of the good.  There is no reason we cannot improve the code in steps, 
on demand, as the need arises.  This is the essence of agile development.  
YAGNI 

> > At least two of the checks I've already added have complaints that they 
> > don't work on headers.

> 

> 

> That's because you used `isExpansionInMainFile`, which is just not needed 
> there


...and that was because I couldn't write an automated test against header files 
to verify the changes made there.  The whole point of THIS changeset was to 
give me a mechanism for verifying the fixes in a header so that I could address 
those issues in the bug database.

But instead of getting on with fixing it, I've spent 6 weeks waiting for this 
changeset to be reviewed and that discussion has prevented an advancement of 
functionality in the testing framework.

> > While this change was waiting to be reviewed, another check was added that 
> > made changes to headers but had no way to verify them.

> 

> 

> Which check do you mean? Does it need a special treatment of headers or is it 
> just able to make fixes in headers as most other checks?


I already quoted the check in question earlier in this thread; please review 
those messages.


http://reviews.llvm.org/D17482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

As it stands currently, you can't commit a header with `CHECK-MESSAGES` and 
`CHECK-FIXES` lines and have them verified.

That's the whole point of this changeset.

Currently you have to do something very hacky in order to verify changes made 
to headers.


http://reviews.llvm.org/D17482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

I'm sorry to be such a nag and I know you're busy, but

This changeset has been held up for a month and a half. (6 weeks since 
originally posted in http://reviews.llvm.org/D16953)

The change isn't that complicated and there haven't really been any comments 
requiring action on my part, so I don't understand why this is taking so long.


http://reviews.llvm.org/D17482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Squeak


http://reviews.llvm.org/D17482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D17482#372677, @alexfh wrote:

> Sorry for leaving this without attention for a while.
>
> I'm somewhat concerned about this change. It's adding a certain level of 
> complexity, but doesn't cover some less trivial cases like handling of 
> multiple headers.


In the context of a clang-tidy check, what would be gained by verifying changes 
made to multiple headers that isn't already tested by verifying changes made to 
a single header?

> Can you take a look at existing tests and say how many times this feature is 
> going to be used, if it gets added to the testing script?


At least two of the checks I've already added have complaints that they don't 
work on headers.

While this change was waiting to be reviewed, another check was added that made 
changes to headers but had no way to verify them.


http://reviews.llvm.org/D17482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

In http://reviews.llvm.org/D17482#372677, @alexfh wrote:

> Can you [...] say how many times this feature is going to be used, if it gets 
> added to the testing script?


Since you can't verify changes against headers right now, noone has bothered to 
add test cases for their checks against headers.

However, I think a case can be made that every check potentially operates 
against headers and that they should be verified as such.  But, since you can't 
do this in the build right now, noone has written such tests.


http://reviews.llvm.org/D17482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

test/clang-tidy/misc-unused-parameters.cpp currently does a hack to verify 
headers by committing the manually fixed header and diffing it:

  // RUN: echo "static void staticFunctionHeader(int i) {}" > %T/header.h
  // RUN: echo "static void staticFunctionHeader(int  /*i*/) {}" > 
%T/header-fixed.h
  // RUN: %check_clang_tidy %s misc-unused-parameters %t -- -header-filter='.*' 
-- -std=c++11 -fno-delayed-template-parsing
  // RUN: diff %T/header.h %T/header-fixed.h
  
  #include "header.h"
  // CHECK-MESSAGES: header.h:1:38: warning

I think this would be much cleaner with `header.h` as a regularly comitted file 
with `CHECK-MESSAGES` and `CHECK-FIXES` lines in that file.


http://reviews.llvm.org/D17482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Sorry for leaving this without attention for a while.

I'm somewhat concerned about this change. It's adding a certain level of 
complexity, but doesn't cover some less trivial cases like handling of multiple 
headers. Can you take a look at existing tests and say how many times this 
feature is going to be used, if it gets added to the testing script?

Thank you!



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')

The name of the flag might be confusing, since it doesn't have the same meaning 
as the clang-tidy flag with the same name. The clang-tidy `--header-filter` 
flag is a regular expression, but this one is a single header filename.


http://reviews.llvm.org/D17482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Another week without reviews...

These changes were already split from a previous changeset without being 
reviewed properly.

Squeak squeak squeak.


http://reviews.llvm.org/D17482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood added a comment.

Squeak


http://reviews.llvm.org/D17482



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D17482: [clang-tidy] Allow tests to verify changes made to header files

[Richard via cfe-commits]

LegalizeAdulthood created this revision.
LegalizeAdulthood added a reviewer: alexfh.
LegalizeAdulthood added a subscriber: cfe-commits.

This changeset improves check_clang_tidy.py to allow chnages made by a check to 
a header to be verified.

A new argument `--header-filter` allows the test file to specify the header 
that will be modified by the check.

`check_clang_tidy.py` will then:

- Copy the named header to the same temporary directory containing the source 
file
- Scrub `CHECK-FIXES` and `CHECK-FIXES` text from the copied header
- Run the `clang-tidy`  on the scrubbed source files with:
  - `-header-filter` and the name of the copied header file.
  -  `-I .` to specify the temporary directory in the include search path
- Verify messages and fixes on the source file.
- Verify messages and fixes on the header file.
- In the case of failure, report differences on the header and the source file.

http://reviews.llvm.org/D17482

Files:
  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