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

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added inline comments.


================
Comment at: clang/include/clang/Driver/ToolChain.h:384
 
+  /// IsBareMetal - Does this tool chain is a baremetal target.
+  static bool IsBareMetal(const llvm::Triple &);
----------------
barannikov88 wrote:
> Is this a correct sentence? (My English is poor.)
> 
I'll fix it. Somehow I forgot to check them.


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131225/new/

https://reviews.llvm.org/D131225