anonymouspc wrote:
@Sirraide, Thanks for your code review!
I've applied 14/15 code reviews in the recent commits, including:
1. remove `emitFilename()` and wait until we can emit a file-only location.
2. remove wasteful `class SARIFDiagnostic::Node::Option`.
3. set `SARIFDiagnostic::Root.Level` into `Error`
4. move `"X Warnings and Y errors generated."` into
`<sarif-file>.runs.invocations.toolExecutionNotifications`.
5. move `SARIFDiagnostic::/*write-into-file*/` from `~SARIFDiagnostic()` into
`SARIFDiagnostic::writeResult()`.
6. made many style fixes.
I haven’t updated the “forkable node” logic yet (right now it’s
`Message.starts_with("candidate")`), mainly because:
1. The proper long-term fix is definitely "build diag-tree where diag is
provided", not "reassemble diag-tree where diag is consumed" (see #151234).
- This PR focuses on consumer (SARIF data-structure and SARIF serializer).
- This PR does not focus on provider (diag constructor).
- The nested/indented diagnostics support is only a first-step. It's
modern, although it requires huge change on existing clang codes. (I'm very
interested in it too, but it should be another PR.).
2. Once we implemented a diag-tree in provider side, the consumer side will
only need a small tweak
```cpp
// >>>>>>>>>>>> now
if (Message.starts_with("candidate"))
CurrentNode = &CurrentNode->getForkableParent();
// <<<<<<<<<<<< future
if (Nesting <= CurrentNode->Nesting)
CurrentNode = &CurrentNode->Parent();
```
to work ok. The diag provider and diag consumer here are only loosely
coupled here, and I believe this is a small step toward nested diagnostics.
-----
Could you please review this PR further?
https://github.com/llvm/llvm-project/pull/174106
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
