[PATCH] D109701: [clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thanks for your patience with this review! I'm currently in WG14 meetings this week and out on vacation next week, so my review availability is a bit limited at the moment (sorry for that).

I think this is heading in the right direction, but there are some lifetime issues we need to figure out how to resolve. I pointed out a few such places, but I did not do an exhaustive search to catch them all.



================
Comment at: clang/include/clang/Basic/Sarif.h:80-82
+  static SarifArtifactLocation create(StringRef URI) {
+    return SarifArtifactLocation{URI};
+  }
----------------
One thing that's worth calling out is that `StringRef` is non-owning which means that the argument passed to create the `SarifArtifactLocation` has to outlive the returned object to avoid dangling references. This makes the class a bit more dangerous to use because `Twine` or automatic `std::string` objects may cause lifetime concerns.

Should these classes be storing a `std::string` so that the memory is owned by SARIF?


================
Comment at: clang/include/clang/Basic/Sarif.h:116
+  static SarifArtifact create(const SarifArtifactLocation &Loc) {
+    return SarifArtifactLocation{Loc};
+  }
----------------
`Loc` is already a `SarifArtifactLocation`, did you mean `SarifArtifact` by any chance?

(Note, this suggests to me we should mark the ctor's here as `explicit`.)


================
Comment at: clang/include/clang/Basic/Sarif.h:229-230
+/// used to create an empty shell onto which attributes can be added using the
+/// \c setX(...) methods. The
+///
+/// For example:
----------------



================
Comment at: clang/include/clang/Basic/Sarif.h:243-245
+  // While this cannot be negative, since this type needs to be serialized
+  // to JSON, it needs to be `int64_t`. The best we can do is assert that
+  // a negative value isn't used to create it
----------------
This comment looks incorrect to me.


================
Comment at: clang/include/clang/Basic/Sarif.h:367
+
+  /// Associate the given rule with the current run
+  ///
----------------
It'd be good to explain why `createRule()` returns a value here and above.


================
Comment at: clang/include/clang/Basic/Sarif.h:378
+  /// There must be a run associated with the document, failing to do so will
+  /// cause undefined behaviour
+  /// \pre
----------------



================
Comment at: clang/include/clang/Basic/SourceLocation.h:441-456
+    bool operator()(const FullSourceLoc &lhs, const FullSourceLoc &rhs) const {
       return lhs.isBeforeInTranslationUnitThan(rhs);
     }
   };
 
   /// Prints information about this FullSourceLoc to stderr.
   ///
----------------
Looks like unrelated formatting changes; feel free to submit these as an NFC change if you'd like, but the changes should be reverted from this patch for clarity.


================
Comment at: clang/lib/Basic/Sarif.cpp:199
+    const SarifArtifactLocation &Location =
+        SarifArtifactLocation::create(FileURI).setIndex(Idx);
+    const SarifArtifact &Artifact = SarifArtifact::create(Location)
----------------
This seems like it'll be a use-after-free because the local `std::string` will be destroyed before the lifetime of the `SarifArtifactLocation` ends.


================
Comment at: clang/lib/Basic/Sarif.cpp:207-209
+    if (statusIter.second) {
+      I = statusIter.first;
+    }
----------------
vaibhav.y wrote:
> aaron.ballman wrote:
> > Our usual coding style elides these too. Btw, you can find the coding style document at: https://llvm.org/docs/CodingStandards.html
> Thanks, sorry there's so many of these! I definitely need to not auto-pilot with style.
No worries! Our style is not... all that typical... so it can be hard to remember.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109701