https://github.com/steakhal commented:
> Overall LPGTM (looks plausibly good to me), but I'm not familiar enough with > the intricacies of template specializations to verify whether the logic is > correct. Well, before I started fixing clang::suppress, I thought I know templates. I was wrong. Now I no longer claim I know them. I only trust tests now. > I didn't read the testcases yet because their sheer amount is daunting and > there are no delimiters and very few explanatory comments among them. This time, I figured I'll generate wast amount of tests, because I don't trust anyone and anything with templates and the lexical decl context. The idea is simple: assume that every primary template, or specialization gets instantiated. Then if the `warnIfReached` is within a curly-braced block that has `suppress`, then it shouldn't surface. Otherwise you should `expect` the warning. So checking the test outcomes is actually fairly simple. The more difficult part is knowing what scenarios were checked. A couple notable one: - forward declarations (has some or not) - define in-class/out class. - full specialization, partial specialization - template within template - template within non-template - friend function, or template Then take the product of these (matrix) and you see the testing problem. > If I understand it correctly the significant entrypoints are the functions > named `instantiate_...`, but this is not immediately obvious -- it would be > nice to have clear separators (e.g. a line of 80 slashes) after each such > function. `instantiate_` stuff are the entry point for the path-sensitive analysis, but other than that it plays no role in the tests. > Also, I would be grateful to have a few comment lines describing the > significance of each testcase -- these should appear just below the delimiter > line to help with understanding the code that follows them. There were actually, but I decided to drop them because code speak, comments rot. And sometimes spelling out the scenario is longer than the test itself. For example: forward declared free-function template friend declared in class, defined outside of class. Or forward declared free-function template friend declared in class, defined inside of class for the T=int* specialization, but otherwise defined outside of class. I could revisit and add some, if you still believe they would help. I could create multiple test files, that's also an option. > I would prefer if you wrote these descriptions without AI use, because I > don't trust AI summaries and I highly trust that you would highlight the most > relevant factors -- but you have more experience with claude, so I can accept > it if you are confident in it. I wrote the commit message. It seems I get too convincing if I don't make grammar mistakes. > By the way, if you see any redundant testcases, it would be nice to remove > them -- in this situation I feel that less is more. Although I'm usually good > with abstractions, these testcases are so abstract and similar, that they > blend together in my mind and interfere with understanding each other. I was thinking about this, but I opted for keeping them because sometimes they list a group of cases, and a couple of them might have been covered before, but now they are together as well. I don't have a strong opinion about this, but adding more definitely felt easier than going over all the existing tests to find the pairs of the ones I wanted to cover to put them "near" eachother. My opinion is that these tests are regression tests, so it's an infinite box of cases with vetted test expectations. No order, or completeness guarantees. Mostly the test cases that strictly demonstrated some undesired behavior in the past. https://github.com/llvm/llvm-project/pull/183727 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
