[PATCH] D69141: [Remarks] Add support for linking remarks

[Jonas Devlieghere via Phabricator via llvm-commits]

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

Few small comments inline but otherwise this LGTM



================
Comment at: llvm/include/llvm/Remarks/Remark.h:119
+  if (!LHS && RHS)
+    return true;
+  if (LHS && !RHS)
----------------
thegameg wrote:
> JDevlieghere wrote:
> > Why does this not return false?
> This way, we order `None` before a valid `Optional<T>`. For example, remarks with no debug location will appear first.
Sounds like this is worth a comment.


================
Comment at: llvm/include/llvm/Remarks/RemarkLinker.h:92
+  /// Note: once serialized, the string table is cleared.
+  const StringTable &getStringTable() const { return StrTab; }
+  StringTable &getStringTable() { return StrTab; }
----------------
Do you need both as part of the public API? Who's externally modifying the string table?


================
Comment at: llvm/lib/Remarks/RemarkLinker.cpp:108
+      return link(*Section, RemarkFormat);
+    else
+      return Error::success();
----------------
JDevlieghere wrote:
> No else after return
There's still and else after `return Error::success();`.


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

https://reviews.llvm.org/D69141