barannikov88 added inline comments.
================ Comment at: clang/include/clang/Driver/ToolChain.h:388 + /// IsRISCVBareMetal - Does this tool chain is a riscv baremetal target. + static bool IsRISCVBareMetal(const llvm::Triple &); + ---------------- manojgupta wrote: > barannikov88 wrote: > > The ToolChain class is an interface class. It is strange to see such kind > > of methods here. `IsBareMetal` should at least be virtual and overridden in > > concrete implementation of baremetal toolchains. `IsRISCVBareMetal` should > > not be here at all. > > What was wrong with the previous implementation that made you move the > > methods here? > There is a need to check for IsBareMetal and variants. They can't be made > virtual since the checks need to happen before instantiating ToolChain class. > I think moving them to Triple class (Triple.h) is a clearer option. > I think moving them to Triple class (Triple.h) is a clearer option. Is the triple is all that is necessary to decide whether the target is bare metal or not? Sounds interesting, but one may argue that Triple should not know about toolchains (like it should not know about C data type bit widths, for example). What if just add a few switch-cases to Driver::getToolChain as for every other toolchain? Retaining the former static method 'BareMetalToolChain::handlesTarget' is still better in my opinion. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131225/new/ https://reviews.llvm.org/D131225 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
