tra accepted this revision. tra added a comment. This revision is now accepted and ready to land.
LGTM with few nits. ================ Comment at: clang/lib/Driver/Driver.cpp:4191 /// or CUDA architecture. -static StringRef getCanonicalArchString(Compilation &C, - const llvm::opt::DerivedArgList &Args, - StringRef ArchStr, - const llvm::Triple &Triple) { +static Optional<StringRef> +getCanonicalArchString(Compilation &C, const llvm::opt::DerivedArgList &Args, ---------------- Nit. Optional is fine, but we could've just checked returned value for Arch.empty(). ================ Comment at: clang/lib/Driver/Driver.cpp:4288 for (StringRef Arch : llvm::split(Arg->getValue(), ",")) { - if (Arch == StringRef("all")) + if (Arch == StringRef("all")) { Archs.clear(); ---------------- Nit: do we need explicit `StringRef()` here? If implicit type conversion does not work, we could use `Arch.equals("all")`. The current code is fine, but it does stick out a bit and makes me wonder if there's something interesting going on there. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135791/new/ https://reviews.llvm.org/D135791 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits