wenlei marked 4 inline comments as done. wenlei added inline comments.
================ Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.h:272 + else + VectLibrary = Impl.ClVectorLibrary; + ---------------- tejohnson wrote: > 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. Ok, I moved it to .cpp, thanks! 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
