[PATCH] D108301: [MSP430][Clang] Update hard-coded MCU data

[Anton Korobeynikov via Phabricator via cfe-commits]

asl requested changes to this revision.
asl added a comment.
This revision now requires changes to proceed.

Please see comments inline



================
Comment at: clang/lib/Driver/ToolChains/MSP430.cpp:43
+/// std::lower_bound is used to perform an efficient binary search on the data.
+static MCUData loadMCUData(const StringRef MCU) {
+  const MCUData MSP430MCUData[612] = {
----------------
maybe it would be better to use `get` instead of `load` here?


================
Comment at: clang/lib/Driver/ToolChains/MSP430.cpp:77
+static void processHWMultFeatures(const Driver &D, const ArgList &Args,
+                                  std::vector<StringRef> &Features,
+                                  StringRef SupportedHWMult) {
----------------
`Features` is effectively an output argument here. Can it be last argument? Maybe it would be better to name `addHWMultFeatures`, overall the function just adds them and do not touch existing things in the `Features` vector?


================
Comment at: clang/lib/Driver/ToolChains/MSP430.cpp:110
 
-  if (MCU && SupportedHWMult == "none")
-    D.Diag(clang::diag::warn_drv_msp430_hwmult_unsupported) << HWMult;
-  if (MCU && HWMult != SupportedHWMult)
-    D.Diag(clang::diag::warn_drv_msp430_hwmult_mismatch)
-        << SupportedHWMult << HWMult;
-
-  if (HWMult == "16bit") {
-    // '16bit' - for 16-bit only hw multiplier.
+  if (HWMult == "16bit")
     Features.push_back("+hwmult16");
----------------
StringSwitch?


================
Comment at: clang/lib/Driver/ToolChains/MSP430.cpp:44
+static MCUData loadMCUData(const StringRef MCU) {
+  const MCUData MSP430MCUData[612] = {
+#define MSP430_MCU(NAME, CPU, HWMULT) {(NAME), (CPU), (HWMULT)},
----------------
Can we simply use `MSP430MCUData[]` here in order not to fix multiple places when new MCU is added?


================
Comment at: clang/lib/Driver/ToolChains/MSP430.cpp:47
 #include "clang/Basic/MSP430Target.def"
-      .Default("none");
-}
+  };
 
----------------
I'd also #undef here for the sake of not polluting everything with extra macros


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108301