[PATCH] D97449: [Diagnose] Unify MCContext and LLVMContext diagnosing

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:477
+  StringRef Message = D.getMessage();
+  if (Message.startswith("error: "))
+    Message = Message.substr(7);
----------------
MaskRay wrote:
> `StringRef::consume_front`
> 
> I know you are moving code, but do you know why it needs to chop off the `error: ` prefix (why does the message get a prefix here?)
It was introduced by https://github.com/llvm/llvm-project/commit/5ec32e7fd845e0b7db33689f33cc2ef7c83710fa. 

I guess it is to canonicalize error messages in case the user just throws in an "error: " prefix like `Ctx.diagnose("error: xxx")` which would give two `error` prefixes otherwise.


================
Comment at: llvm/lib/MC/MCContext.cpp:869
+    SMLoc Loc,
+    std::function<void(SMDiagnostic &, const SourceMgr *)> GetMessage) {
+  SourceMgr SM;
----------------
MaskRay wrote:
> Use lightweight function_ref since you don't need to store the callback.
I was hesitant to do this because it requires including `STLExtras.h` in MCContext.h which could be bad for compile-time. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97449