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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits