skatrak wrote: > Duplicating this comment in every test is ludicrous. At most you should have > a txt file in the top level directory
I understand your concern @arsenm, but also this PR isn't introducing this copy-pasted notice. It's just moving around some previously misplaced tests while following the existing guidelines documented in the README. > The intent behind including the note in each integration test is to encourage > anyone adding a test to first consider whether the same coverage can be > achieved without running all the compiler stages, and to use an integration > test only when necessary. Thanks for the clarification @kiranchandramohan, that was my understanding as well. Seeing how many tests worked around that by being added to Lower instead, and others added to Integration without the notice, it's not looking super effective but I can't think of a better way to do this either. I imagine letting the README be the only place where this is documented would only make it easier to miss. > If there is another way to provide the same reminder when a test is added, or > if integration tests are no more a concern for llvm, or if everyone feels > that duplicating the message is not serving the purpose then we can consider > switching. Maybe an RFC to decide on this issue would be a good idea, since removing all these existing comments (if that's what people agree on) can be done separately afterwards at very little cost. I don't think that this decision should be a blocker for this patch. https://github.com/llvm/llvm-project/pull/215536 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
