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

Mon Aug 8 05:28:22 PDT 2022

barannikov88 added a comment.

In D131225#3706207 <https://reviews.llvm.org/D131225#3706207>, @abidh wrote:

> I think this patch is moving the things in the right direction. But please wait for some other reviewers to chime in.

It seems quite opposite to me.

================
Comment at: clang/include/clang/Driver/ToolChain.h:384

+  /// IsBareMetal - Does this tool chain is a baremetal target.
+  static bool IsBareMetal(const llvm::Triple &);
----------------
Is this a correct sentence? (My English is poor.)

================
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 &);
+
----------------
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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131225