tejohnson added a comment. In D77632#1976308 <https://reviews.llvm.org/D77632#1976308>, @mehdi_amini wrote:
> In D77632#1976231 <https://reviews.llvm.org/D77632#1976231>, @wenlei wrote: > > > And agree with @tejohnson, if the openness is a feature, it should be > > covered in tests, otherwise it can feel somewhat like a loophole and prone > > to breakage > > > The thing is that LLVM does not have much C++ unittests in general, so most > of the "features" like this one that LLVM offers as a library are just an > artifact of being "designed as a library" and being mindful about the > layering. > From this point of view this patch is changing the design of a component > that was modular/pluggable into a closed system. The interfaces being relied on were in the underlying Impl class, I think if that is expected to be pluggable and stable it really needs unit testing to reflect that usage. > I'm perfectly fine with trying to add a unit-test, I just don't know yet > where it would fit in the LLVM testing though. Presumably the testing should be in llvm/unittests/Analysis/TargetLibraryInfoTest.cpp, which already exists but only tests the LibFuncs (builtins) interfaces. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77632/new/ https://reviews.llvm.org/D77632 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
