[PATCH] D58484: [DO NOT SUBMIT] Add -vs-diagnostics.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

In D58484#1407078 <https://reviews.llvm.org/D58484#1407078>, @ikudrin wrote:

> For me, the code should be, above all, reliable. You can trust it only in that case; otherwise, you never know, when and how it is going to be broken and produce an unexpected result.
>
> I would suggest a simple test for both approaches. What if a developer adds a new diagnostic message?
>
> With D58300 <https://reviews.llvm.org/D58300> it will probably just work as expected in both modes out-of-the-box. You even don't need to check that it is properly formatted, because most of the formatting is performed in one place, uniformly.
>
> With this patch (D58484 <https://reviews.llvm.org/D58484>) the developer might not be aware of the existence of the `--vs-diagnostics` switch. Thus, recognizing patterns will not probably be updated and the message generation will be broken for an unpredictable time until someone comes across that new message in Visual Studio.


I think I disagree. The error messages we are printing out is under our control, so we don't have to worry too much about the hypothetical scenario that "what if someone unaware of this does <something>". We know what we are printing, and we'd also like to make it machine-readable in some degree. The number of error messages added for each release is very small (I'd guess less than ten in average), and that's pretty manageable number. In addition to that, even when something goes wrong and some error message fell through this mechanism, it is not an end-of-the-world kind of thing. It is just mildly annoying if you are using an IDE and there's a link error. But I don't think even that would happen.

> To sum up, I consider this patch to be very fragile. It would be hard for me to welcome it.
> 
> I have also noticed the following differences in the generated messages compared to D58300 <https://reviews.llvm.org/D58300>:
> 
> - The line number is not enclosed in parentheses, how it should.

It shouldn't be too hard to fix it.

> - The patch is unable to generate separate messages for each duplicate symbol, which D58300 <https://reviews.llvm.org/D58300> does.

I'd like to first discuss what would be expected as an error message for a duplicate symbol, as IIUC, MSVC doesn't show two separate error messages for two conflicting locations.

> - The patch sometimes uses unsuitable strings for the source location, for example, an offset in a section.
> - In some cases it cannot recognize the source location to be used in the front of the message.

It shouldn't be too hard to fix them, too.

> You may see all these issues with tests from D58300 <https://reviews.llvm.org/D58300>. It looks like trying to fixing them will complicate this patch and make it even more fragile.
> 
> I believe that D58300 <https://reviews.llvm.org/D58300> not only adds VS-compatible diagnostic messages but makes a step towards the better code quality. It separates message formatting in a distinct place, making the other code cleaner. It also adds a possibility to add other formatters in the future, not only for different IDEs but for any other tool.

I'd like to emphasize the cost of reading, understanding and maintaining code. Imagine that you were a first-time reader of the lld source code and looking for a way to print out an error message. The current way of doing it is very easy. It is also extremely easy to identify which line of code corresponds to a specific error message. That readability comes from the fact that we don't do too much clever things when printing out error messages. I can see that abstracting the error message construction to match exiting IDE may seem like the "right" approach, but we should be careful not to overdesign. This is perhaps a "worse is better" situation. If something very simple-minded works just enough, we should take it instead of a more complex and theoretically comprehensive one as a first approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58484