[PATCH] D108942: [compiler-rt] Add more diagnostic to InstrProfError

[Petr Hosek via Phabricator via llvm-commits]

phosek added a comment.

I missed the latest patchset. I have a few more comments, feel free to address those in follow up changes.



================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:153
+  if (!ErrMsg.empty())
+    OS << ": '" << ErrMsg << "'";
+
----------------
I'd omit the extra quotes, that's more consistent with the way Clang and other tools report errors


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:466
+        instrprof_error::malformed,
+        ("counter offset(" + Twine(CounterOffset) + ")" + " is < 0").str());
+
----------------
In error messages, we usually prefer spelling things out fully, so this should be `counter offset X is negative`, I'd omit the parenthesis.


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:470-472
+                 ("counter offset(" + Twine(CounterOffset) + ")" + " > " +
+                  "max number of counters(" + Twine((uint32_t)MaxNumCounters) +
+                  ")")
----------------
Here I'd say `counter offset X is greater that the maximum number of counters Y`.


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:477-481
+                 ("number of counters is out of bounds(counter offset(" +
+                  Twine((uint32_t)CounterOffset) + ") + " +
+                  "number of counters(" + Twine(NumCounters) + ") > " +
+                  "max number of counters(" + Twine((uint32_t)MaxNumCounters) +
+                  "))")
----------------
Similarly here I'd say just `number of counters X is greater than the maximum number of counters Y` where X would be a sum of CounterOffset and NumCounters, no need to report those separately since you already check the offset above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108942