[PATCH] D84673: [clang][cli] Port DiagnosticOpts to new option parsing system

Mon Jan 11 21:41:06 PST 2021

rupprecht added a comment.

I ran into this commit when integrating commits today (specifically, 97100646d1b4526de1eac3aacdb0b098739c6ec9 <https://reviews.llvm.org/rG97100646d1b4526de1eac3aacdb0b098739c6ec9>) -- there's nothing wrong with this patch AFAICT, but I'm wondering if the error messaging/handling could be improved somehow.

tl;dr I reduced an example in D94468 <https://reviews.llvm.org/D94468>

We have an internal user creating a ToolInvocation and attaching a DiagnosticConsumer to it. They were also using `-ferror-limit=-1`, which if I understand now, may be meaningless -- `-ferror-limit=0` is used to mean "unlimited error messages", which is probably what they meant.

Anyway, with the DiagnosticConsumer attached, the test crashed with an assertion failure `"Assertion 'SourceMgr && "SourceManager not set!"` failed.", which really wasn't helpful at all. After much longer than I care to admit, I thought to remove the `setDiagnosticConsumer`, and was able to find the `"invalid integral value '-1' in '-ferror-limit -1'"` error in the logs, and I was able to find the real issue pretty quickly after that.

I don't think the usage was out of the ordinary, so I created D94468 <https://reviews.llvm.org/D94468> as an example if you have time to take a look, to save the next person that may run into this issue.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84673