aaron.ballman added inline comments.

================
Comment at: clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:31
   std::string OutputFile;
+  const Preprocessor &PP;
 
----------------
You don't really need the whole preprocessor, just the language options, right? 
If so, why not thread the language options through rather than the PP?


================
Comment at: clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:150
+                                    unsigned int TokenLen = 0) {
+  assert(!Loc.isInvalid());
+
----------------
Can you add some text to the assertion to explain what's gone wrong if the 
assert triggers? e.g. `assert(!Loc.isInvalid() && "invalid location when 
adjusting column position");` or something (same for the others asserts you've 
added)?


================
Comment at: clang/test/Analysis/diagnostics/sarif-multi-diagnostic-test.c:34
+int unicode()
+{
+  int løçål = 0;
----------------
Formatting of the curly brace looks wrong.


================
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}}
+}
----------------
What file encoding is used by this file (and is there a BOM)? Have you tried 
this test on multiple platforms by any chance?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70689



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to