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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM! I think we've had sufficient time for other reviewers to lodge concerns and we can deal with any other issues post-commit. Do you need me to commit on your behalf?



================
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) &&
----------------
jranieri-grammatech wrote:
> aaron.ballman wrote:
> > 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?
> Good catch that this can't return NULL.
> 
> I tested out defining things on the command line, but this code deals with expansion locations and wasn't affected by it. Unless you can think of a way to reproduce it, I think I'd rather pass in the `invalid` argument and assert that it isn't invalid.
> I think I'd rather pass in the invalid argument and assert that it isn't invalid.

That's fine by me -- I can't think of another way to reproduce it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70689