manojgupta 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: > 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. > >Is the triple is all that is necessary to decide whether the target is bare >metal or not? At least for baremetal, from what I see, triple seems to be the way how baremetal toolchain is decided in practice. Examples: 1. https://gitweb.gentoo.org/proj/crossdev.git/tree/crossdev#n331 ( # Bare metal targets *-newlib|*-elf|*-eabi|*-rtems*) 2. https://elinux.org/images/1/15/Anatomy_of_Cross-Compilation_Toolchains.pdf ( arm-foo-none-eabi, bare-metal toolchain targeting the ARM architecture, from vendor foo ) 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
