rahmanl added a comment. In D85408#2266559 <https://reviews.llvm.org/D85408#2266559>, @dblaikie wrote:
> In D85408#2262134 <https://reviews.llvm.org/D85408#2262134>, @rahmanl wrote: > >> @efriedma Would you please chime in specially with respect to @MaskRay 's >> comment about the clang test involving assembly? > > I'll chime in, perhaps @efriedma will/can too: This patch has no changes to > Clang's code, so it should have no changes to Clang's tests, generally > speaking. > > Whatever codepath/behavior is being tested by that Clang test change should > be testable (& generally only tested) via LLVM's tools/tests, like llc, I > would expect? (I guess that's what the basic-block-sections-labels.ll test is > doing? Making the Clang test update redundant) > > The only reason I'd expect to see the assembly tested in Clang is if the flag > that enables this feature is an MCOptions (or other similar API level > feature) member, rather than part of LLVM IR proper. In that case the only > way to test that the flag has any effect (since we can't observe the > MCOptions struct state directly in a test) is to test for the MCOptions > effect on LLVM's output. Depending on the complexity, sometimes such tests > are just skipped entirely (leaving the setting of the MCOption untested) - I > think I did that for, for example, DWARF type units. Other times, an > end-to-end test is used to ensure the flag is working. But that's all the > test should do: test the flag is working, with some very basic/rudimentary > property of the feature being enabled (so pretty much /any/ change to the way > the feature works will still pass the test, but if the feature were turned > off entirely (the MCOptions flag stopped being set correctly), the test would > fail). Not test all the functionality of the feature that the flag enables - > all that testing should be down in LLVM. Thanks for the clear explanation @dblaikie. In light of your comment, I removed the complex assembly from the clang test, since the LLVM tests are already testing those. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85408/new/ https://reviews.llvm.org/D85408 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
