vaibhav.y added inline comments.
================ Comment at: clang/include/clang/Basic/Sarif.h:167 + + ThreadFlow setRange(const CharSourceRange &ItemRange) { + Range = ItemRange; ---------------- aaron.ballman wrote: > Should we assert this source range is not a token range? I don't have a strong opinion here (since these are validated downstream in `createPhysicalLocation`) but it makes sense to be defensive & assert early. I'll preserve the downstream one as well in case a new type gets added that feeds data into `createPhysicalLocation` as well. ================ Comment at: clang/include/clang/Basic/Sarif.h:280 + SarifResult setLocations(llvm::ArrayRef<CharSourceRange> DiagLocs) { + Locations.assign(DiagLocs.begin(), DiagLocs.end()); + return *this; ---------------- aaron.ballman wrote: > In an asserts build, should we additionally have a loop to assert that each > location is a char range rather than a token range? Will do, I had the asserts in `createPhysicalLocation` initially. Adding them at the site of creation makes sense as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109701/new/ https://reviews.llvm.org/D109701 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits