[PATCH] D128490: [ODRHash diagnostics] Transform method `ASTReader::diagnoseOdrViolations` into a class `ODRDiagsEmitter`. NFC.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai planned changes to this revision.
vsapsai added inline comments.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:9577-9584
+static std::string getOwningModuleNameForDiagnostic(const Decl *D) {
+  // If we know the owning module, use it.
+  if (Module *M = D->getImportedOwningModule())
+    return M->getFullModuleName();
+
+  // Not from a module.
+  return {};
----------------
ChuanqiXu wrote:
> We add a static method with the same name of the original one but we don't remove the original one. It looks odd. Could we remove the odd one?
Hmm, I've tried removing the old one `ASTReader::getOwningModuleNameForDiagnostic` but some tests were failing. Now all the tests seem to be fine. I'll remove `ASTReader::getOwningModuleNameForDiagnostic` and we'll see if pre-commit tests are happy.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:9733-9734
+    bool Diagnosed = false;
+    ODRDiagsEmitter DiagsEmitter(Diags, getContext(),
+                                 getPreprocessor().getLangOpts());
+    CXXRecordDecl *FirstRecord = Merge.first;
----------------
ChuanqiXu wrote:
> Could we hoist these construction?
Sure. I see your point that `ODRDiagsEmitter` is stateless regarding `diagnoseMismatch` methods, so repeating the same construction looks excessive and verbose. But if anybody later has any reasons to change that, it should be possible as `ODRDiagsEmitter` construction is cheap.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128490