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 <https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it> > > 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
