[PATCH] D135791: [Clang] Do not crash when an invalid offload architecture is set

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.


================
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,
----------------
tra wrote:
> Nit. Optional is fine, but we could've just checked returned value for Arch.empty().
Probably the better option. I originally used `Optional` for the implicit boolean conversion, but I don't think it'll make the code that much more complicated.


================
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();
----------------
tra wrote:
> 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.
You're right, previously this used `Arg-getValue()` but then when it changed to `llvm::split` it got converted to a `StringRef` so this can be updated now. Thanks for pointing it out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135791