[clang] fixes header processing for `-fdiagnostics-format=sarif`

[cjdb]

Summary

Including headers used to fire an assertion; now they report a
diagnostic similarly to unstructured diagnostics.

Depends on D146654.

[cjdb]

actually commits the files

[cjdb]

fixes something that wasn't supposed to be changed in the rebase

[aaron.ballman]

It looks like there's a valid precommit CI failure that needs to be addressed:

********************
Failed Tests (1):
  Clang :: Frontend/sarif-diagnostics.cpp

clang/lib/Frontend/SARIFDiagnostic.cpp

217–234

Why do we want -1 as the rule ID and... can we use "-1" instead of doing a string conversion?

[cjdb]

sorts artifacts so that they're output in index order

[cjdb]

This comment has been deleted.

clang/lib/Basic/Sarif.cpp

314–317

I'm wondering if I should instead copy CurrentArtifacts to a vector and sort prior to insertion, rather than in post.

clang/lib/Frontend/SARIFDiagnostic.cpp

217–234

lol at obvious C++ goof.

Re -1, there doesn't seem to be a diagnostic associated with this note, so I picked a value that I know isn't in use.

[cjdb]

fixes string goof

[aaron.ballman]

clang/lib/Basic/Sarif.cpp

314–317

I think this is okay, but might be interesting to see what has better perf. Given that this is 1) related to issuing diagnostics, and 2) dumping data onto disk, I am not super concerned about perf for this until we see something show up in a profiler.

clang/lib/Frontend/SARIFDiagnostic.cpp

217–234

Rather than have these functions devise their own diagnostic IDs, should we make some SARIF-specific notes in the diagnostics system that we can use more directly? (Might be overkill for the first such note here, but the other emitFooLocation() functions make me think we're going to want this wrapped in a helper sooner rather than later.)

[cjdb]

clang/lib/Frontend/SARIFDiagnostic.cpp

217–234

Since it's entirely string-based, we can probably evolve a naming scheme for these over time. The numbers are currently useless, so I'm okay with having "-1" for now though. We should come up with a strategy on whether or not we want Microsoft-like codes, but I'm pretty against that (and I think my opinions are on the record, but happy to reiterate).

[aaron.ballman]

(Note, precommit CI on Windows still shows a valid failure.)

clang/lib/Frontend/SARIFDiagnostic.cpp

217–234

We discussed this offline a bit and yeah, we're going to have to come up with some sort of naming scheme for these eventually. For the moment, -1 is fine for this one use case, but you should probably add a FIXME comment about needing to replace the -1 with something better in the future.

[cjdb]

fixes breakage and adds FIXME

[cjdb]

swaps commits to see if that fixes CI (part 1)

[cjdb]

And on this CL's side too

[aaron.ballman]

In D145201#4214395, @cjdb wrote:

swaps commits to see if that fixes CI (part 1)

Oh, if this is just stacked patches confusing our precommit CI, then it's okay. I hadn't realized this was a stacked patch.

[aaron.ballman]

LGTM aside from a tiny variable naming nit I missed before.

clang/lib/Basic/Sarif.cpp

314–317

[cjdb]

In D145201#4216567, @aaron.ballman wrote:

In D145201#4214395, @cjdb wrote:

swaps commits to see if that fixes CI (part 1)

Oh, if this is just stacked patches confusing our precommit CI, then it's okay. I hadn't realized this was a stacked patch.

No, I introduced a new patch and it was weirdly failing everything, so I decided to see if it could be merged before this one.