[PATCH] D69141: [Remarks] Add support for linking remarks
Jonas Devlieghere via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 31 09:14:31 PDT 2019
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
More information about the llvm-commits
mailing list