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 &); + ---------------- barannikov88 wrote: > 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. > They can't be made virtual since the checks need to happen before > instantiating ToolChain class. This is kind of two different things. It can be made virtual. The name of the method `IsBareMetal` of the `ToolChain` class suggests that it is checking whether //this concrete instance// of the ToolChain is baremetal. This (virtual) method could be used in getCompilerRTPath, for example. To //instantiate// BareMetalToolchain you need another function (like the former BareMetalToolChain::handlesTarget, or the suggested approach with Triple). For these instantiations `IsBareMetal` will return true, and false for all other instantiations which are not bare metal. 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
