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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
