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

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG246c5a994b75: [ODRHash diagnostics] Transform method 
`ASTReader::diagnoseOdrViolations` into… (authored by vsapsai).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128490

Files:
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Serialization/ASTReader.cpp

Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -9186,19 +9186,6 @@
   }
 }
 
-std::string ASTReader::getOwningModuleNameForDiagnostic(const Decl *D) {
-  // If we know the owning module, use it.
-  if (Module *M = D->getImportedOwningModule())
-return M->getFullModuleName();
-
-  // Otherwise, use the name of the top-level module the decl is within.
-  if (ModuleFile *M = getOwningModuleFile(D))
-return M->ModuleName;
-
-  // Not from a module.
-  return {};
-}
-
 void ASTReader::finishPendingActions() {
   while (!PendingIdentifierInfos.empty() || !PendingFunctionTypes.empty() ||
  !PendingIncompleteDeclChains.empty() || !PendingDeclChains.empty() ||
@@ -9446,6 +9433,114 @@
   PendingMergedDefinitionsToDeduplicate.clear();
 }
 
+namespace clang {
+class ODRDiagsEmitter {
+public:
+  ODRDiagsEmitter(DiagnosticsEngine , const ASTContext ,
+  const LangOptions )
+  : Diags(Diags), Context(Context), LangOpts(LangOpts) {}
+
+  /// Diagnose ODR mismatch between 2 FunctionDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  bool diagnoseMismatch(const FunctionDecl *FirstFunction,
+const FunctionDecl *SecondFunction) const;
+
+  /// Diagnose ODR mismatch between 2 EnumDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  bool diagnoseMismatch(const EnumDecl *FirstEnum,
+const EnumDecl *SecondEnum) const;
+
+  /// Diagnose ODR mismatch between 2 CXXRecordDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  /// To compare 2 declarations with merged and identical definition data
+  /// you need to provide pre-merge definition data in \p SecondDD.
+  bool
+  diagnoseMismatch(const CXXRecordDecl *FirstRecord,
+   const CXXRecordDecl *SecondRecord,
+   const struct CXXRecordDecl::DefinitionData *SecondDD) const;
+
+  /// Get the best name we know for the module that owns the given
+  /// declaration, or an empty string if the declaration is not from a module.
+  static std::string getOwningModuleNameForDiagnostic(const Decl *D);
+
+private:
+  using DeclHashes = llvm::SmallVector, 4>;
+
+  // Used with err_module_odr_violation_mismatch_decl and
+  // note_module_odr_violation_mismatch_decl
+  // This list should be the same Decl's as in ODRHash::isDeclToBeProcessed
+  enum ODRMismatchDecl {
+EndOfClass,
+PublicSpecifer,
+PrivateSpecifer,
+ProtectedSpecifer,
+StaticAssert,
+Field,
+CXXMethod,
+TypeAlias,
+TypeDef,
+Var,
+Friend,
+FunctionTemplate,
+Other
+  };
+
+  struct DiffResult {
+const Decl *FirstDecl = nullptr, *SecondDecl = nullptr;
+ODRMismatchDecl FirstDiffType = Other, SecondDiffType = Other;
+  };
+
+  // If there is a diagnoseable difference, FirstDiffType and
+  // SecondDiffType will not be Other and FirstDecl and SecondDecl will be
+  // filled in if not EndOfClass.
+  static DiffResult FindTypeDiffs(DeclHashes ,
+  DeclHashes );
+
+  DiagnosticBuilder Diag(SourceLocation Loc, unsigned DiagID) const {
+return Diags.Report(Loc, DiagID);
+  }
+
+  // Use this to diagnose that an unexpected Decl was encountered
+  // or no difference was detected. This causes a generic error
+  // message to be emitted.
+  void diagnoseSubMismatchUnexpected(DiffResult ,
+ const NamedDecl *FirstRecord,
+ StringRef FirstModule,
+ const NamedDecl *SecondRecord,
+ StringRef SecondModule) const;
+
+  void diagnoseSubMismatchDifferentDeclKinds(DiffResult ,
+ const NamedDecl *FirstRecord,
+ StringRef FirstModule,
+ const NamedDecl *SecondRecord,
+ StringRef SecondModule) const;
+
+  bool diagnoseSubMismatchField(const NamedDecl *FirstRecord,
+StringRef FirstModule, StringRef SecondModule,
+const FieldDecl *FirstField,
+const FieldDecl *SecondField) const;
+
+  bool 

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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 457723.
vsapsai added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128490

Files:
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Serialization/ASTReader.cpp

Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -9186,19 +9186,6 @@
   }
 }
 
-std::string ASTReader::getOwningModuleNameForDiagnostic(const Decl *D) {
-  // If we know the owning module, use it.
-  if (Module *M = D->getImportedOwningModule())
-return M->getFullModuleName();
-
-  // Otherwise, use the name of the top-level module the decl is within.
-  if (ModuleFile *M = getOwningModuleFile(D))
-return M->ModuleName;
-
-  // Not from a module.
-  return {};
-}
-
 void ASTReader::finishPendingActions() {
   while (!PendingIdentifierInfos.empty() || !PendingFunctionTypes.empty() ||
  !PendingIncompleteDeclChains.empty() || !PendingDeclChains.empty() ||
@@ -9446,6 +9433,114 @@
   PendingMergedDefinitionsToDeduplicate.clear();
 }
 
+namespace clang {
+class ODRDiagsEmitter {
+public:
+  ODRDiagsEmitter(DiagnosticsEngine , const ASTContext ,
+  const LangOptions )
+  : Diags(Diags), Context(Context), LangOpts(LangOpts) {}
+
+  /// Diagnose ODR mismatch between 2 FunctionDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  bool diagnoseMismatch(const FunctionDecl *FirstFunction,
+const FunctionDecl *SecondFunction) const;
+
+  /// Diagnose ODR mismatch between 2 EnumDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  bool diagnoseMismatch(const EnumDecl *FirstEnum,
+const EnumDecl *SecondEnum) const;
+
+  /// Diagnose ODR mismatch between 2 CXXRecordDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  /// To compare 2 declarations with merged and identical definition data
+  /// you need to provide pre-merge definition data in \p SecondDD.
+  bool
+  diagnoseMismatch(const CXXRecordDecl *FirstRecord,
+   const CXXRecordDecl *SecondRecord,
+   const struct CXXRecordDecl::DefinitionData *SecondDD) const;
+
+  /// Get the best name we know for the module that owns the given
+  /// declaration, or an empty string if the declaration is not from a module.
+  static std::string getOwningModuleNameForDiagnostic(const Decl *D);
+
+private:
+  using DeclHashes = llvm::SmallVector, 4>;
+
+  // Used with err_module_odr_violation_mismatch_decl and
+  // note_module_odr_violation_mismatch_decl
+  // This list should be the same Decl's as in ODRHash::isDeclToBeProcessed
+  enum ODRMismatchDecl {
+EndOfClass,
+PublicSpecifer,
+PrivateSpecifer,
+ProtectedSpecifer,
+StaticAssert,
+Field,
+CXXMethod,
+TypeAlias,
+TypeDef,
+Var,
+Friend,
+FunctionTemplate,
+Other
+  };
+
+  struct DiffResult {
+const Decl *FirstDecl = nullptr, *SecondDecl = nullptr;
+ODRMismatchDecl FirstDiffType = Other, SecondDiffType = Other;
+  };
+
+  // If there is a diagnoseable difference, FirstDiffType and
+  // SecondDiffType will not be Other and FirstDecl and SecondDecl will be
+  // filled in if not EndOfClass.
+  static DiffResult FindTypeDiffs(DeclHashes ,
+  DeclHashes );
+
+  DiagnosticBuilder Diag(SourceLocation Loc, unsigned DiagID) const {
+return Diags.Report(Loc, DiagID);
+  }
+
+  // Use this to diagnose that an unexpected Decl was encountered
+  // or no difference was detected. This causes a generic error
+  // message to be emitted.
+  void diagnoseSubMismatchUnexpected(DiffResult ,
+ const NamedDecl *FirstRecord,
+ StringRef FirstModule,
+ const NamedDecl *SecondRecord,
+ StringRef SecondModule) const;
+
+  void diagnoseSubMismatchDifferentDeclKinds(DiffResult ,
+ const NamedDecl *FirstRecord,
+ StringRef FirstModule,
+ const NamedDecl *SecondRecord,
+ StringRef SecondModule) const;
+
+  bool diagnoseSubMismatchField(const NamedDecl *FirstRecord,
+StringRef FirstModule, StringRef SecondModule,
+const FieldDecl *FirstField,
+const FieldDecl *SecondField) const;
+
+  bool diagnoseSubMismatchTypedef(const NamedDecl *FirstRecord,
+  StringRef FirstModule, StringRef SecondModule,
+  const TypedefNameDecl 

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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In D128490#3689958 , @ChuanqiXu wrote:

> LGTM then.

Thanks for the review! Sorry for not answering earlier, I was on vacation. Now 
I'll rebase the change and will test it again before committing.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu accepted this revision.
ChuanqiXu added a comment.
This revision is now accepted and ready to land.

LGTM then.


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

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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai marked 2 inline comments as done.
vsapsai added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:9193-9195
-  // Otherwise, use the name of the top-level module the decl is within.
-  if (ModuleFile *M = getOwningModuleFile(D))
-return M->ModuleName;

At first I though this part was important for certain tests. But I guess I've 
messed up somewhere else.


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

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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 448710.
vsapsai added a comment.

- Address review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128490

Files:
  clang/include/clang/AST/DeclCXX.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/Serialization/ASTReader.cpp

Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -9185,19 +9185,6 @@
   }
 }
 
-std::string ASTReader::getOwningModuleNameForDiagnostic(const Decl *D) {
-  // If we know the owning module, use it.
-  if (Module *M = D->getImportedOwningModule())
-return M->getFullModuleName();
-
-  // Otherwise, use the name of the top-level module the decl is within.
-  if (ModuleFile *M = getOwningModuleFile(D))
-return M->ModuleName;
-
-  // Not from a module.
-  return {};
-}
-
 void ASTReader::finishPendingActions() {
   while (!PendingIdentifierInfos.empty() || !PendingFunctionTypes.empty() ||
  !PendingIncompleteDeclChains.empty() || !PendingDeclChains.empty() ||
@@ -9445,6 +9432,114 @@
   PendingMergedDefinitionsToDeduplicate.clear();
 }
 
+namespace clang {
+class ODRDiagsEmitter {
+public:
+  ODRDiagsEmitter(DiagnosticsEngine , const ASTContext ,
+  const LangOptions )
+  : Diags(Diags), Context(Context), LangOpts(LangOpts) {}
+
+  /// Diagnose ODR mismatch between 2 FunctionDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  bool diagnoseMismatch(const FunctionDecl *FirstFunction,
+const FunctionDecl *SecondFunction) const;
+
+  /// Diagnose ODR mismatch between 2 EnumDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  bool diagnoseMismatch(const EnumDecl *FirstEnum,
+const EnumDecl *SecondEnum) const;
+
+  /// Diagnose ODR mismatch between 2 CXXRecordDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  /// To compare 2 declarations with merged and identical definition data
+  /// you need to provide pre-merge definition data in \p SecondDD.
+  bool
+  diagnoseMismatch(const CXXRecordDecl *FirstRecord,
+   const CXXRecordDecl *SecondRecord,
+   const struct CXXRecordDecl::DefinitionData *SecondDD) const;
+
+  /// Get the best name we know for the module that owns the given
+  /// declaration, or an empty string if the declaration is not from a module.
+  static std::string getOwningModuleNameForDiagnostic(const Decl *D);
+
+private:
+  using DeclHashes = llvm::SmallVector, 4>;
+
+  // Used with err_module_odr_violation_mismatch_decl and
+  // note_module_odr_violation_mismatch_decl
+  // This list should be the same Decl's as in ODRHash::isDeclToBeProcessed
+  enum ODRMismatchDecl {
+EndOfClass,
+PublicSpecifer,
+PrivateSpecifer,
+ProtectedSpecifer,
+StaticAssert,
+Field,
+CXXMethod,
+TypeAlias,
+TypeDef,
+Var,
+Friend,
+FunctionTemplate,
+Other
+  };
+
+  struct DiffResult {
+const Decl *FirstDecl = nullptr, *SecondDecl = nullptr;
+ODRMismatchDecl FirstDiffType = Other, SecondDiffType = Other;
+  };
+
+  // If there is a diagnoseable difference, FirstDiffType and
+  // SecondDiffType will not be Other and FirstDecl and SecondDecl will be
+  // filled in if not EndOfClass.
+  static DiffResult FindTypeDiffs(DeclHashes ,
+  DeclHashes );
+
+  DiagnosticBuilder Diag(SourceLocation Loc, unsigned DiagID) const {
+return Diags.Report(Loc, DiagID);
+  }
+
+  // Use this to diagnose that an unexpected Decl was encountered
+  // or no difference was detected. This causes a generic error
+  // message to be emitted.
+  void diagnoseSubMismatchUnexpected(DiffResult ,
+ const NamedDecl *FirstRecord,
+ StringRef FirstModule,
+ const NamedDecl *SecondRecord,
+ StringRef SecondModule) const;
+
+  void diagnoseSubMismatchDifferentDeclKinds(DiffResult ,
+ const NamedDecl *FirstRecord,
+ StringRef FirstModule,
+ const NamedDecl *SecondRecord,
+ StringRef SecondModule) const;
+
+  bool diagnoseSubMismatchField(const NamedDecl *FirstRecord,
+StringRef FirstModule, StringRef SecondModule,
+const FieldDecl *FirstField,
+const FieldDecl *SecondField) const;
+
+  bool diagnoseSubMismatchTypedef(const NamedDecl *FirstRecord,
+  StringRef FirstModule, StringRef SecondModule,
+  

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu 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 {};

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?



Comment at: clang/lib/Serialization/ASTReader.cpp:9733-9734
+bool Diagnosed = false;
+ODRDiagsEmitter DiagsEmitter(Diags, getContext(),
+ getPreprocessor().getLangOpts());
+CXXRecordDecl *FirstRecord = Merge.first;

Could we hoist these construction?


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

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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

OK. Now I've reverted mostly to the old indentation, preserving some of the 
quirks. Hope it is easier to review now.


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

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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 447899.
vsapsai added a comment.

- Revert indentation for the second and subsequent lines in lambda-now-method 
calls.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128490

Files:
  clang/include/clang/AST/DeclCXX.h
  clang/lib/Serialization/ASTReader.cpp

Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -9445,6 +9445,110 @@
   PendingMergedDefinitionsToDeduplicate.clear();
 }
 
+namespace clang {
+class ODRDiagsEmitter {
+public:
+  ODRDiagsEmitter(DiagnosticsEngine , const ASTContext ,
+  const LangOptions )
+  : Diags(Diags), Context(Context), LangOpts(LangOpts) {}
+
+  /// Diagnose ODR mismatch between 2 FunctionDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  bool diagnoseMismatch(const FunctionDecl *FirstFunction,
+const FunctionDecl *SecondFunction) const;
+
+  /// Diagnose ODR mismatch between 2 EnumDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  bool diagnoseMismatch(const EnumDecl *FirstEnum,
+const EnumDecl *SecondEnum) const;
+
+  /// Diagnose ODR mismatch between 2 CXXRecordDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  /// To compare 2 declarations with merged and identical definition data
+  /// you need to provide pre-merge definition data in \p SecondDD.
+  bool
+  diagnoseMismatch(const CXXRecordDecl *FirstRecord,
+   const CXXRecordDecl *SecondRecord,
+   const struct CXXRecordDecl::DefinitionData *SecondDD) const;
+
+private:
+  using DeclHashes = llvm::SmallVector, 4>;
+
+  // Used with err_module_odr_violation_mismatch_decl and
+  // note_module_odr_violation_mismatch_decl
+  // This list should be the same Decl's as in ODRHash::isDeclToBeProcessed
+  enum ODRMismatchDecl {
+EndOfClass,
+PublicSpecifer,
+PrivateSpecifer,
+ProtectedSpecifer,
+StaticAssert,
+Field,
+CXXMethod,
+TypeAlias,
+TypeDef,
+Var,
+Friend,
+FunctionTemplate,
+Other
+  };
+
+  struct DiffResult {
+const Decl *FirstDecl = nullptr, *SecondDecl = nullptr;
+ODRMismatchDecl FirstDiffType = Other, SecondDiffType = Other;
+  };
+
+  // If there is a diagnoseable difference, FirstDiffType and
+  // SecondDiffType will not be Other and FirstDecl and SecondDecl will be
+  // filled in if not EndOfClass.
+  static DiffResult FindTypeDiffs(DeclHashes ,
+  DeclHashes );
+
+  DiagnosticBuilder Diag(SourceLocation Loc, unsigned DiagID) const {
+return Diags.Report(Loc, DiagID);
+  }
+
+  // Use this to diagnose that an unexpected Decl was encountered
+  // or no difference was detected. This causes a generic error
+  // message to be emitted.
+  void diagnoseSubMismatchUnexpected(DiffResult ,
+ const NamedDecl *FirstRecord,
+ StringRef FirstModule,
+ const NamedDecl *SecondRecord,
+ StringRef SecondModule) const;
+
+  void diagnoseSubMismatchDifferentDeclKinds(DiffResult ,
+ const NamedDecl *FirstRecord,
+ StringRef FirstModule,
+ const NamedDecl *SecondRecord,
+ StringRef SecondModule) const;
+
+  bool diagnoseSubMismatchField(const NamedDecl *FirstRecord,
+StringRef FirstModule, StringRef SecondModule,
+const FieldDecl *FirstField,
+const FieldDecl *SecondField) const;
+
+  bool diagnoseSubMismatchTypedef(const NamedDecl *FirstRecord,
+  StringRef FirstModule, StringRef SecondModule,
+  const TypedefNameDecl *FirstTD,
+  const TypedefNameDecl *SecondTD,
+  bool IsTypeAlias) const;
+
+  bool diagnoseSubMismatchVar(const NamedDecl *FirstRecord,
+  StringRef FirstModule, StringRef SecondModule,
+  const VarDecl *FirstVD,
+  const VarDecl *SecondVD) const;
+
+private:
+  DiagnosticsEngine 
+  const ASTContext 
+  const LangOptions 
+};
+} // namespace clang
+
 static unsigned computeODRHash(QualType Ty) {
   ODRHash Hasher;
   Hasher.AddQualType(Ty);
@@ -9470,6 +9574,15 @@
   return Hasher.CalculateHash();
 }
 
+static std::string getOwningModuleNameForDiagnostic(const Decl *D) {
+  // If we know the owning module, use it.
+  if (Module *M = D->getImportedOwningModule())
+ 

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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 447892.
vsapsai added a comment.

- Replicate some creative old indentation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128490

Files:
  clang/include/clang/AST/DeclCXX.h
  clang/lib/Serialization/ASTReader.cpp

Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -9445,6 +9445,110 @@
   PendingMergedDefinitionsToDeduplicate.clear();
 }
 
+namespace clang {
+class ODRDiagsEmitter {
+public:
+  ODRDiagsEmitter(DiagnosticsEngine , const ASTContext ,
+  const LangOptions )
+  : Diags(Diags), Context(Context), LangOpts(LangOpts) {}
+
+  /// Diagnose ODR mismatch between 2 FunctionDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  bool diagnoseMismatch(const FunctionDecl *FirstFunction,
+const FunctionDecl *SecondFunction) const;
+
+  /// Diagnose ODR mismatch between 2 EnumDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  bool diagnoseMismatch(const EnumDecl *FirstEnum,
+const EnumDecl *SecondEnum) const;
+
+  /// Diagnose ODR mismatch between 2 CXXRecordDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  /// To compare 2 declarations with merged and identical definition data
+  /// you need to provide pre-merge definition data in \p SecondDD.
+  bool
+  diagnoseMismatch(const CXXRecordDecl *FirstRecord,
+   const CXXRecordDecl *SecondRecord,
+   const struct CXXRecordDecl::DefinitionData *SecondDD) const;
+
+private:
+  using DeclHashes = llvm::SmallVector, 4>;
+
+  // Used with err_module_odr_violation_mismatch_decl and
+  // note_module_odr_violation_mismatch_decl
+  // This list should be the same Decl's as in ODRHash::isDeclToBeProcessed
+  enum ODRMismatchDecl {
+EndOfClass,
+PublicSpecifer,
+PrivateSpecifer,
+ProtectedSpecifer,
+StaticAssert,
+Field,
+CXXMethod,
+TypeAlias,
+TypeDef,
+Var,
+Friend,
+FunctionTemplate,
+Other
+  };
+
+  struct DiffResult {
+const Decl *FirstDecl = nullptr, *SecondDecl = nullptr;
+ODRMismatchDecl FirstDiffType = Other, SecondDiffType = Other;
+  };
+
+  // If there is a diagnoseable difference, FirstDiffType and
+  // SecondDiffType will not be Other and FirstDecl and SecondDecl will be
+  // filled in if not EndOfClass.
+  static DiffResult FindTypeDiffs(DeclHashes ,
+  DeclHashes );
+
+  DiagnosticBuilder Diag(SourceLocation Loc, unsigned DiagID) const {
+return Diags.Report(Loc, DiagID);
+  }
+
+  // Use this to diagnose that an unexpected Decl was encountered
+  // or no difference was detected. This causes a generic error
+  // message to be emitted.
+  void diagnoseSubMismatchUnexpected(DiffResult ,
+ const NamedDecl *FirstRecord,
+ StringRef FirstModule,
+ const NamedDecl *SecondRecord,
+ StringRef SecondModule) const;
+
+  void diagnoseSubMismatchDifferentDeclKinds(DiffResult ,
+ const NamedDecl *FirstRecord,
+ StringRef FirstModule,
+ const NamedDecl *SecondRecord,
+ StringRef SecondModule) const;
+
+  bool diagnoseSubMismatchField(const NamedDecl *FirstRecord,
+StringRef FirstModule, StringRef SecondModule,
+const FieldDecl *FirstField,
+const FieldDecl *SecondField) const;
+
+  bool diagnoseSubMismatchTypedef(const NamedDecl *FirstRecord,
+  StringRef FirstModule, StringRef SecondModule,
+  const TypedefNameDecl *FirstTD,
+  const TypedefNameDecl *SecondTD,
+  bool IsTypeAlias) const;
+
+  bool diagnoseSubMismatchVar(const NamedDecl *FirstRecord,
+  StringRef FirstModule, StringRef SecondModule,
+  const VarDecl *FirstVD,
+  const VarDecl *SecondVD) const;
+
+private:
+  DiagnosticsEngine 
+  const ASTContext 
+  const LangOptions 
+};
+} // namespace clang
+
 static unsigned computeODRHash(QualType Ty) {
   ODRHash Hasher;
   Hasher.AddQualType(Ty);
@@ -9470,6 +9574,15 @@
   return Hasher.CalculateHash();
 }
 
+static std::string getOwningModuleNameForDiagnostic(const Decl *D) {
+  // If we know the owning module, use it.
+  if (Module *M = D->getImportedOwningModule())
+return M->getFullModuleName();
+
+  // 

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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 447882.
vsapsai added a comment.

- Who could have suspected that some lambdas were double-indented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128490

Files:
  clang/include/clang/AST/DeclCXX.h
  clang/lib/Serialization/ASTReader.cpp

Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -9445,6 +9445,110 @@
   PendingMergedDefinitionsToDeduplicate.clear();
 }
 
+namespace clang {
+class ODRDiagsEmitter {
+public:
+  ODRDiagsEmitter(DiagnosticsEngine , const ASTContext ,
+  const LangOptions )
+  : Diags(Diags), Context(Context), LangOpts(LangOpts) {}
+
+  /// Diagnose ODR mismatch between 2 FunctionDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  bool diagnoseMismatch(const FunctionDecl *FirstFunction,
+const FunctionDecl *SecondFunction) const;
+
+  /// Diagnose ODR mismatch between 2 EnumDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  bool diagnoseMismatch(const EnumDecl *FirstEnum,
+const EnumDecl *SecondEnum) const;
+
+  /// Diagnose ODR mismatch between 2 CXXRecordDecl.
+  ///
+  /// Returns true if found a mismatch and diagnosed it.
+  /// To compare 2 declarations with merged and identical definition data
+  /// you need to provide pre-merge definition data in \p SecondDD.
+  bool
+  diagnoseMismatch(const CXXRecordDecl *FirstRecord,
+   const CXXRecordDecl *SecondRecord,
+   const struct CXXRecordDecl::DefinitionData *SecondDD) const;
+
+private:
+  using DeclHashes = llvm::SmallVector, 4>;
+
+  // Used with err_module_odr_violation_mismatch_decl and
+  // note_module_odr_violation_mismatch_decl
+  // This list should be the same Decl's as in ODRHash::isDeclToBeProcessed
+  enum ODRMismatchDecl {
+EndOfClass,
+PublicSpecifer,
+PrivateSpecifer,
+ProtectedSpecifer,
+StaticAssert,
+Field,
+CXXMethod,
+TypeAlias,
+TypeDef,
+Var,
+Friend,
+FunctionTemplate,
+Other
+  };
+
+  struct DiffResult {
+const Decl *FirstDecl = nullptr, *SecondDecl = nullptr;
+ODRMismatchDecl FirstDiffType = Other, SecondDiffType = Other;
+  };
+
+  // If there is a diagnoseable difference, FirstDiffType and
+  // SecondDiffType will not be Other and FirstDecl and SecondDecl will be
+  // filled in if not EndOfClass.
+  static DiffResult FindTypeDiffs(DeclHashes ,
+  DeclHashes );
+
+  DiagnosticBuilder Diag(SourceLocation Loc, unsigned DiagID) const {
+return Diags.Report(Loc, DiagID);
+  }
+
+  // Use this to diagnose that an unexpected Decl was encountered
+  // or no difference was detected. This causes a generic error
+  // message to be emitted.
+  void diagnoseSubMismatchUnexpected(DiffResult ,
+ const NamedDecl *FirstRecord,
+ StringRef FirstModule,
+ const NamedDecl *SecondRecord,
+ StringRef SecondModule) const;
+
+  void diagnoseSubMismatchDifferentDeclKinds(DiffResult ,
+ const NamedDecl *FirstRecord,
+ StringRef FirstModule,
+ const NamedDecl *SecondRecord,
+ StringRef SecondModule) const;
+
+  bool diagnoseSubMismatchField(const NamedDecl *FirstRecord,
+StringRef FirstModule, StringRef SecondModule,
+const FieldDecl *FirstField,
+const FieldDecl *SecondField) const;
+
+  bool diagnoseSubMismatchTypedef(const NamedDecl *FirstRecord,
+  StringRef FirstModule, StringRef SecondModule,
+  const TypedefNameDecl *FirstTD,
+  const TypedefNameDecl *SecondTD,
+  bool IsTypeAlias) const;
+
+  bool diagnoseSubMismatchVar(const NamedDecl *FirstRecord,
+  StringRef FirstModule, StringRef SecondModule,
+  const VarDecl *FirstVD,
+  const VarDecl *SecondVD) const;
+
+private:
+  DiagnosticsEngine 
+  const ASTContext 
+  const LangOptions 
+};
+} // namespace clang
+
 static unsigned computeODRHash(QualType Ty) {
   ODRHash Hasher;
   Hasher.AddQualType(Ty);
@@ -9470,6 +9574,15 @@
   return Hasher.CalculateHash();
 }
 
+static std::string getOwningModuleNameForDiagnostic(const Decl *D) {
+  // If we know the owning module, use it.
+  if (Module *M = D->getImportedOwningModule())
+return 

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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In D128490#3670543 , @ChuanqiXu wrote:

> For these indentation changes, maybe it is a good idea to edit the 
> `.git-blame-ignore-revs` files in the root path.

That is a good idea. I'm thinking about a different option: I make my change 
but keep `ODRDiagsEmitter` methods over-indented to preserve the old 
indentation. The code is moved to a different file in D128695 
 and I just format the entire file. This way 
the current change is easier to review (now and in git history) and the 
formatting is [eventually] correct. As for me, having good `git blame` is more 
important than having perfect formatting (proposed formatting will be still OK).


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D128490#3670441 , @vsapsai wrote:

> Sorry for the huge amount of changes in ASTReader.cpp but most of them are 
> indentation changes, so I hope it's not too bad. I've tried to minimize the 
> changes unrelated to introducing `ODRDiagsEmitter` but if anybody has other 
> ideas/requests, I'm totally willing to reduce the load on reviewers.

For these indentation changes, maybe it is a good idea to edit the 
`.git-blame-ignore-revs` files in the root path.


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

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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Sorry for the huge amount of changes in ASTReader.cpp but most of them are 
indentation changes, so I hope it's not too bad. I've tried to minimize the 
changes unrelated to introducing `ODRDiagsEmitter` but if anybody has other 
ideas/requests, I'm totally willing to reduce the load on reviewers.


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