JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land.
LGTM with some nits inline. ================ Comment at: llvm/docs/Remarks.rst:630 +example, LLVM IR passes will emit ``llvm::DiagnosticInfoOptimization*`` that +get converted to ``llvm::remarks::Remark`` objects to be serialized to disk, +and clang could set up its own specialized remark streamer that takes ---------------- This sentence is pretty long and complex. Maybe add a full stop instead of the comma and start a new sentence. ================ Comment at: llvm/docs/Remarks.rst:644 + +With the only disadvantage being making the code more complex. + ---------------- I think the trade-off is clear, but if you really want to be explicit maybe just say at the cost of an extra layer of abstraction. ================ Comment at: llvm/include/llvm/IR/LLVMContext.h:225 - /// Return the streamer used by the backend to save remark diagnostics. If it - /// does not exist, diagnostics are not saved in a file but only emitted via - /// the diagnostic handler. - RemarkStreamer *getRemarkStreamer(); - const RemarkStreamer *getRemarkStreamer() const; - - /// Set the diagnostics output used for optimization diagnostics. - /// This filename may be embedded in a section for tools to find the - /// diagnostics whenever they're needed. + /// The "Main remark streamer" used by all the specialized remark streamers. + /// This streamer keeps generic remark metadata in memory throughout the life ---------------- why is Main capitalized but not remark streamer? I think this should all be lowercase? That would be consistent with the rest of the patch. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73676/new/ https://reviews.llvm.org/D73676 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
