alexfh added a comment. In http://reviews.llvm.org/D17482#387457, @LegalizeAdulthood wrote:
> 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. My main concern is that this change makes the already complicated and unobvious testing mechanism even more complicated and even less obvious for a very little (as I see it now) benefit. The added functionality supports a very limited use case (a single header) 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). There's also an outstanding comment that you didn't seem to have addressed yet. In http://reviews.llvm.org/D17482#378685, @LegalizeAdulthood wrote: > 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. That's because you used `isExpansionInMainFile`, which is just not needed there (BTW, I believe, the usages of `isExpansionInMainFile` in SimplifyBooleanExprCheck.cpp are not needed as well. At most they work around some other issues - incorrect template handling, for example.). Most checks don't need to do anything special to work on headers, they don't need to know whether a location is in the main file or not, and they don't need special tests for handling headers correctly. Adding such tests "just in case" will only add work to check authors and make the process more complicated for no reason. There are a few exceptions (DefinitionsInHeadersCheck.cpp, UnusedAliasDeclsCheck.cpp, UnnamedNamespaceInHeaderCheck.cpp, IncludeOrderCheck.cpp, GlobalNamesInHeadersCheck.cpp, that's about it), and only two of them (IncludeOrderCheck and DefinitionsInHeadersCheck) actually need special tests for fixes in 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. 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? ================ Comment at: test/clang-tidy/modernize-redundant-void-arg.h:1 @@ +1,2 @@ +#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 ---------------- Additional headers should be in the Inputs/ directory to prevent more pollution of the test/clang-tidy directory. http://reviews.llvm.org/D17482 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
