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

Sergei Barannikov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Aug 8 11:31:21 PDT 2022


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



More information about the cfe-commits mailing list