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