agozillon marked 4 inline comments as done. agozillon added a comment. Addressed 4/5 of your comments in the latest patch @awarzynski, I've replied to the final one to give a little more information to add to the final decision you make!
================ Comment at: flang/lib/Frontend/CompilerInvocation.cpp:730 + res.getLangOpts().OMPHostIRFile = arg->getValue(); + if (!llvm::sys::fs::exists(res.getLangOpts().OMPHostIRFile)) + diags.Report(clang::diag::err_drv_omp_host_ir_file_not_found) ---------------- awarzynski wrote: > I think that this is expecting a bit too much from `CompilerInvocation`. > Instead, whatever is going to use this file should complain if the file does > not exist (`CompilerInvocation` is merely a messenger here, and file I/O can > be quite expensive, hence my reservations). > > Is it possible to create a test for "invalid file path" wherever this becomes > relevant? I believe the follow up patch I have here which uses the file, emits an error if it can't find it: https://reviews.llvm.org/D148370 However, in this case it's an assert rather than more useful Diagnostics unfortunately. Although, this segment of code does currently just mimic what Clang does in it's own `CompilerInvocation`: https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/CompilerInvocation.cpp#L3883 So it depends if we wish to try to maintain the status quo for the shared argument across the compiler frontends! So whichever you prefer, I am happy to remove the check at this level and let the lowering handle the problem if it arises :-) but I do like sharing the commonality with Clang where possible. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148038/new/ https://reviews.llvm.org/D148038 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
