[PATCH] D70689: [analyzer] Fix SARIF column locations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: dblaikie, echristo, xbolva00.
aaron.ballman marked an inline comment as done.
aaron.ballman added a comment.

Mostly LG aside from a question about an assertion. Adding other reviewers outside of GrammaTech in case there are other concerns we've missed.



================
Comment at: clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:157
+  const MemoryBuffer *Buf = SM.getBuffer(LocInfo.first);
+  assert(Buf && "got a NULL buffer for the location's file");
+  assert(Buf->getBufferSize() >= (LocInfo.second + TokenLen) &&
----------------
I don't think this API is able to return a null buffer, but it can return an invalid buffer under legitimate circumstances, I believe. For instance, if the source location is in the scratch space used for things like macros defined on the command line with `-D`. I think you should pass the optional `invalid` parameter to the call to `getBuffer()` and then do no adjustments if the buffer is invalid. WDYT?


================
Comment at: clang/test/Analysis/diagnostics/sarif-multi-diagnostic-test.c:35-36
+{
+  int løçål = 0;
+  /* ☃ */ return 1 / løçål; // expected-warning {{Division by zero}}
+}
----------------
jranieri-grammatech wrote:
> aaron.ballman wrote:
> > What file encoding is used by this file (and is there a BOM)? Have you tried this test on multiple platforms by any chance?
> The file is encoded in UTF-8 without a BOM. I have not tried the test on other platforms.
Okay, I think that's reasonable, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70689