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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
