tejohnson added inline comments.
================ Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.h:272 + else + VectLibrary = Impl.ClVectorLibrary; + ---------------- wenlei wrote: > tejohnson wrote: > > Suggest moving the implementation of this constructor to the .cpp file, in > > which case you can just set VectLibrary directly from ClVectorLibrary there > > and remove the member on the Impl object. > There're utilities that use `TargetLibraryInfo`, but don't link with > `TargetLibraryInfo.o`. And looking at `TargetLibraryInfo`, all of the > functions are in this header, so I assumed it's intentional to keep this type > self-contained in this header, as it's public API, which is why I add > `ClVectorLibrary` to Impl to pass it back to `TargetLibraryInfo`. For > `TargetLibraryInfoImpl`, it's ok to have the implementation outside of the > header. I can give it a try if keeping the class implementation/definition > self-contained in the header isn't important. I don't think there should be anything using TLI without linking with libAnalysis, which contains TargetLibraryInfo.o. I don't think it should be important to keep the implementation in the header, any more so than for other headers in the Analysis library. 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
