[PATCH] D131225: Driver: Refactor and support per target dirs in baremetal

[Sergei Barannikov via Phabricator via llvm-commits]

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