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 applied the “forkable node” logic yet, 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. At now, I listed a huge "forkable" diagID table in `static
std::unordered_set<unsigned> SARIFDiagnostic::ForkableDiagIDs = {...}`. This is
only a temporary measure.
3. In the future, once we implemented a diag-tree in provider side, the
consumer side will only need a small tweak
```cpp
// >>>>>>>>>>>> now
if (ForkableDiagIDs.contains(DiagID))
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
