[PATCH] D52776: [OptRemarks] Add library for parsing optimization remarks

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

Just one more round of comments.

Other than that, I think that this is good.



================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:47-48
+    StringRef Function;
+    /// Optional.
+    StringRef File;
+    Optional<unsigned> Line;
----------------
If `File` is optional, then why not make it an `Optional<StringRef>` to make it clear?


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:54
+    ParseState(SmallVectorImpl<LLVMOptRemarkArg> &Args) : Args(&Args) {}
+    /// Use TmpArgs only as a **temporary** buffer.
+    ~ParseState() { Args->clear(); }
----------------
Is this comment out of date?  I don't see `TmpArgs` referenced anywhere in here.


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:91-92
+  static void HandleDiagnostic(const SMDiagnostic &Diag, void *Ctx) {
+    auto *Parser = static_cast<RemarkParser *>(Ctx);
+    Diag.print(/*ProgName=*/nullptr, Parser->ErrorStream, /*ShowColors*/ false,
+               /*ShowKindLabels*/ true);
----------------
Is `Parser` (and I guess `Ctx`) guaranteed to not be a `nullptr`? If not, I can see `Parser->ErrorStream` causing some issues.


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:135
+  if (!Value)
+    return make_error<ParseError>("expecting a value of scalar type.", Node);
+  Result = Value->getRawValue();
----------------
Should this an similar occurrences be "expected" rather than "expecting"? Errors are usually past-tense.


================
Comment at: lib/OptRemarks/OptRemarksParser.cpp:234-235
+
+  if (ValueStr.empty() || KeyStr.empty())
+    return make_error<ParseError>("value or key missing.", *ArgMap);
+
----------------
Maybe split this into two errors to make it clear which one is missing?


https://reviews.llvm.org/D52776