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

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added a comment.

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.

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

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58484