anonymouspc wrote:
> Thank you for the fix! I think you should add a release note to
> `clang/docs/ReleaseNotes.rst` so users know about the fixed crash. I think it
> probably also makes sense to add a test case to
> `clang/unittests/Basic/SarifTest.cpp` showing the behavior of include
> locations and demonstrating the fix.
Sure!
- I added a note to the release notes.
- I modified the `SarifDocumentWriterTest.checkSerializingArtifacts` test in
`clang/unittests/Basic/SarifTest.cpp`. The `SarifDocumentWriterTest` now checks
the serialization of both `Location` and the new `relatedLocation`.
- *It seems that the existing tests of `SarifTest.cpp` focuses on "whether
the `SarifDocumentWriter` can correctly serialize a given message", instead of
"how we produce a new diagnostic", So I simply added the same `DiagLoc` (as the
`Location` one above) to the `relatedLocation` here as well.*
- *Currently the `relatedLocation` always points to an "In file included
from".*
- *In the future (when clang supports nested/indented results, as both GCC
and MSVC has already supported nesting/indentation in sarif mode), the
`relatedLocation` can also refer to a common source range as well.*
Welcome for code review, thank you! Happy post-thanksgiving. :)
https://github.com/llvm/llvm-project/pull/170415
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
