[PATCH] D88394: [Driver][M68k] (Patch 8/8) Add driver support for M68k

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added a comment.

Some very minor nits - the 68060 omission is the biggest one (apple might not have used it but commodore did!)



================
Comment at: clang/include/clang/Driver/Options.td:155
+def m_m68k_Features_Group: OptionGroup<"<m68k features group>">,
+                             Group<m_Group>, DocName<"M68k">;
 def m_ppc_Features_Group : OptionGroup<"<ppc features group>">,
----------------
(sorting) put this before mips?


================
Comment at: clang/lib/Driver/ToolChains/Arch/M68k.cpp:44
+
+    llvm::Regex CPUPattern("m?680([01234]{1})0");
+    llvm::SmallVector<StringRef, 2> Matches;
----------------
Why no 68060 ?


================
Comment at: clang/lib/Driver/ToolChains/Arch/M68k.cpp:64
+    return "M68040";
+  }
+
----------------
(style) remove unnecessary braces?


================
Comment at: clang/lib/Driver/ToolChains/Arch/M68k.cpp:72
+                                     const ArgList &Args,
+                                     std::vector<StringRef> &Features) {
+
----------------
(clang-format) indentation?


================
Comment at: clang/lib/Driver/ToolChains/Arch/M68k.cpp:94
+    ABI = m68k::FloatABI::Hard;
+  }
+
----------------
remove braces


================
Comment at: clang/lib/Driver/ToolChains/Arch/M68k.h:35
+                             const llvm::opt::ArgList &Args,
+                             std::vector<llvm::StringRef> &Features);
+
----------------
(clang-format) indentation?


================
Comment at: clang/lib/Driver/ToolChains/Arch/M68k.h:38
+} // end namespace m68k
+} // namespace tools
+} // end namespace driver
----------------
(clang-tidy) end namespace tools



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:380
+    m68k::getM68kTargetFeatures(D, Triple, Args, Features);
+    break;
   }
----------------
(sorting) move before msp430 ?


================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:297
+  case llvm::Triple::m68k:
+    return m68k::getM68kTargetCPU(Args);
+
----------------
(sorting) move down to before mips ?


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

https://reviews.llvm.org/D88394