plotfi marked 3 inline comments as done. plotfi added a comment. In D60974#1534662 <https://reviews.llvm.org/D60974#1534662>, @rupprecht wrote:
> Can you upload this patch with context? Either use arc or upload w/ -U99999 Thanks for the review. > I seem to have a lot of comments, but they're all nits -- my major concern > being the `llvm_unreachable`s should be errors instead (i.e. should be > triggered in release builds). > > Since this is clearly labeled as experimental, I think you should feel free > to commit if you can get another lgtm (@compnerd?) ================ Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3614-3616 + Args.hasArg(options::OPT_iterface_stub_version_EQ) + ? Args.getLastArgValue(options::OPT_iterface_stub_version_EQ) + : "") ---------------- rupprecht wrote: > `Args.getLastArgValue(options::OPT_iterface_stub_version_EQ)` should already > default to returning the empty string if unset (note the default param) But what if it is set to something that's not either experimental-yaml-elf-v1 or experimental-tapi-elf-v1? This was to truncate any values that aren't those two to "" so that the error check could just be if (StubFormat.empty()). Maybe something other than a string switch would have been more explicit. ================ Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1688 + if (!ProgramActionPair.second) + llvm_unreachable("Must specify a valid interface stub format."); + Opts.ProgramAction = ProgramActionPair.first; ---------------- rupprecht wrote: > I think this is very much reachable if someone provides > `--interface-stub-version=x` AH yes, you are right. This should probably be a diag. (clang/test/InterfaceStubs/bad-format.cpp tests for this "unreachable"). ================ Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:41 + return true; + if (Symbols.find(ND) != Symbols.end()) + return true; ---------------- rupprecht wrote: > llvm::is_contained(Symbols, ND) llvm::is_contained does not appear to work with std::map. I will try to figure out why. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60974/new/ https://reviews.llvm.org/D60974 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits