Fznamznon added inline comments.
================
Comment at: clang/lib/Driver/Driver.cpp:2571
// No driver mode exposes -x and /TC or /TP; we don't support mixing them.
assert(!Args.hasArg(options::OPT_x) && "-x and /TC or /TP is not allowed");
}
----------------
MaskRay wrote:
> Fznamznon wrote:
> > MaskRay wrote:
> > > ```
> > > - assert(!Args.hasArg(options::OPT_x) && "-x and /TC or /TP is not
> > > allowed");
> > > + if (auto *A = Args.getLastArg(options::OPT_x))
> > > + Diag(diag::err_drv_unsupported_opt_with_suggestion)
> > > + << A->getAsString(Args) << "/TC' or '/TP";
> > > ```
> > Thank you, the suggestion seems reasonable to me. Although, the concrete
> > place doesn't.
> > The suggested line to change is under if (we have /TC or /TP argument), so
> > if I put error emission there it won't be emitted if no /TC or /TP was
> > passed and the original problem reported in
> > https://github.com/llvm/llvm-project/issues/59307 won't be resolved. People
> > will still be confused by the warning saying `-x c` passed after last input
> > when in fact they passed `/clang:-x` before the input.
> >
> > Also, the whole function doesn't return even if diagnostic is emitted, so I
> > put error emission in a way to not emit a warning saying that -x is passed
> > after last input together with it, because both errors emitted like this:
> >
> > error: '-x c' after last input file has no effect
> > error: unsupported option '-x c'; did you mean '/TC' or '/TP'?
> >
> > look confusing.
> You need to remove `assert(!Args.hasArg(options::OPT_x) && "-x and /TC or /TP
> is not allowed");` as otherwise `/clang:-x /TC` leads to a crash in an assert
> build of clang (`LLVM_ENABLE_ASSERTIONS=on`).
Ok, removed the assert. Verified there is no crash in the test.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142757/new/
https://reviews.llvm.org/D142757
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
