[PATCH] D154459: [ODRHash] Stop hashing `ObjCMethodDecl::isOverriding` as it doesn't capture inherent method quality.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: ChuanqiXu, jansvoboda11, Bigcheese.
Herald added a subscriber: ributzka.
Herald added a project: All.
vsapsai requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

`isOverriding` depends on the surrounding code and not on the method
itself. That's why it can be different in different modules. And
mismatches shouldn't be an error.

rdar://109481753


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D154459

Files:
  clang/lib/AST/ODRDiagsEmitter.cpp
  clang/lib/AST/ODRHash.cpp
  clang/test/Modules/compare-objc-nonisolated-methods.m


Index: clang/test/Modules/compare-objc-nonisolated-methods.m
===
--- /dev/null
+++ clang/test/Modules/compare-objc-nonisolated-methods.m
@@ -0,0 +1,54 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// Test that different values of `ObjCMethodDecl::isOverriding` in different 
modules
+// is not an error because it depends on the surrounding code and not on the 
method itself.
+// RUN: %clang_cc1 -fsyntax-only -verify -I%t/include -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t/modules.cache 
-fmodule-name=Override %t/test-overriding.m
+
+//--- include/Common.h
+@interface NSObject
+@end
+
+//--- include/Indirection.h
+#import 
+
+//--- include/module.modulemap
+module Common {
+  header "Common.h"
+  export *
+}
+module Indirection {
+  header "Indirection.h"
+  export *
+}
+module Override {
+  header "Override.h"
+  export *
+}
+
+//--- include/Override.h
+#import 
+@interface SubClass: NSObject
+- (void)potentialOverride;
+@end
+
+//--- Override_Internal.h
+#import 
+@interface NSObject(InternalCategory)
+- (void)potentialOverride;
+@end
+
+//--- test-overriding.m
+//expected-no-diagnostics
+// Get non-modular version of `SubClass`, so that `-[SubClass 
potentialOverride]`
+// is an override of a method in `InternalCategory`.
+#import "Override_Internal.h"
+#import 
+
+// Get modular version of `SubClass` where `-[SubClass potentialOverride]` is
+// not an override because module "Override" doesn't know about 
Override_Internal.h.
+#import 
+
+void triggerOverrideCheck(SubClass *sc) {
+  [sc potentialOverride];
+}
Index: clang/lib/AST/ODRHash.cpp
===
--- clang/lib/AST/ODRHash.cpp
+++ clang/lib/AST/ODRHash.cpp
@@ -377,7 +377,6 @@
 Hash.AddBoolean(Method->isVariadic());
 Hash.AddBoolean(Method->isSynthesizedAccessorStub());
 Hash.AddBoolean(Method->isDefined());
-Hash.AddBoolean(Method->isOverriding());
 Hash.AddBoolean(Method->isDirectMethod());
 Hash.AddBoolean(Method->isThisDeclarationADesignatedInitializer());
 Hash.AddBoolean(Method->hasSkippedBody());
Index: clang/lib/AST/ODRDiagsEmitter.cpp
===
--- clang/lib/AST/ODRDiagsEmitter.cpp
+++ clang/lib/AST/ODRDiagsEmitter.cpp
@@ -2096,7 +2096,8 @@
   << FirstDecl->getSourceRange();
   Diag(SecondDecl->getLocation(),
diag::note_module_odr_violation_mismatch_decl_unknown)
-  << SecondModule << FirstDiffType << SecondDecl->getSourceRange();
+  << SecondModule.empty() << SecondModule << FirstDiffType
+  << SecondDecl->getSourceRange();
   return true;
 }
 


Index: clang/test/Modules/compare-objc-nonisolated-methods.m
===
--- /dev/null
+++ clang/test/Modules/compare-objc-nonisolated-methods.m
@@ -0,0 +1,54 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// Test that different values of `ObjCMethodDecl::isOverriding` in different modules
+// is not an error because it depends on the surrounding code and not on the method itself.
+// RUN: %clang_cc1 -fsyntax-only -verify -I%t/include -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache -fmodule-name=Override %t/test-overriding.m
+
+//--- include/Common.h
+@interface NSObject
+@end
+
+//--- include/Indirection.h
+#import 
+
+//--- include/module.modulemap
+module Common {
+  header "Common.h"
+  export *
+}
+module Indirection {
+  header "Indirection.h"
+  export *
+}
+module Override {
+  header "Override.h"
+  export *
+}
+
+//--- include/Override.h
+#import 
+@interface SubClass: NSObject
+- (void)potentialOverride;
+@end
+
+//--- Override_Internal.h
+#import 
+@interface NSObject(InternalCategory)
+- (void)potentialOverride;
+@end
+
+//--- test-overriding.m
+//expected-no-diagnostics
+// Get non-modular version of `SubClass`, so that `-[SubClass potentialOverride]`
+// is an override of a method in `InternalCategory`.
+#import "Override_Internal.h"
+#import 
+
+// Get modular version of `SubClass` where `-[SubClass potentialOverride]` is
+// not an override because module "Override" doesn't know about Override_Internal.h.
+#import 
+
+void triggerOverrideCheck(SubClass *sc) {
+  [sc potentialOverride];
+}
Index: clang/lib

[PATCH] D154459: [ODRHash] Stop hashing `ObjCMethodDecl::isOverriding` as it doesn't capture inherent method quality.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Thanks for the review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154459

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

[PATCH] D154459: [ODRHash] Stop hashing `ObjCMethodDecl::isOverriding` as it doesn't capture inherent method quality.

[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 rG18530e5d0770: [ODRHash] Stop hashing 
`ObjCMethodDecl::isOverriding` as it doesn't capture… (authored by 
vsapsai).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154459

Files:
  clang/lib/AST/ODRDiagsEmitter.cpp
  clang/lib/AST/ODRHash.cpp
  clang/test/Modules/compare-objc-nonisolated-methods.m


Index: clang/test/Modules/compare-objc-nonisolated-methods.m
===
--- /dev/null
+++ clang/test/Modules/compare-objc-nonisolated-methods.m
@@ -0,0 +1,54 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// Test that different values of `ObjCMethodDecl::isOverriding` in different 
modules
+// is not an error because it depends on the surrounding code and not on the 
method itself.
+// RUN: %clang_cc1 -fsyntax-only -verify -I%t/include -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t/modules.cache 
-fmodule-name=Override %t/test-overriding.m
+
+//--- include/Common.h
+@interface NSObject
+@end
+
+//--- include/Indirection.h
+#import 
+
+//--- include/module.modulemap
+module Common {
+  header "Common.h"
+  export *
+}
+module Indirection {
+  header "Indirection.h"
+  export *
+}
+module Override {
+  header "Override.h"
+  export *
+}
+
+//--- include/Override.h
+#import 
+@interface SubClass: NSObject
+- (void)potentialOverride;
+@end
+
+//--- Override_Internal.h
+#import 
+@interface NSObject(InternalCategory)
+- (void)potentialOverride;
+@end
+
+//--- test-overriding.m
+//expected-no-diagnostics
+// Get non-modular version of `SubClass`, so that `-[SubClass 
potentialOverride]`
+// is an override of a method in `InternalCategory`.
+#import "Override_Internal.h"
+#import 
+
+// Get modular version of `SubClass` where `-[SubClass potentialOverride]` is
+// not an override because module "Override" doesn't know about 
Override_Internal.h.
+#import 
+
+void triggerOverrideCheck(SubClass *sc) {
+  [sc potentialOverride];
+}
Index: clang/lib/AST/ODRHash.cpp
===
--- clang/lib/AST/ODRHash.cpp
+++ clang/lib/AST/ODRHash.cpp
@@ -377,7 +377,6 @@
 Hash.AddBoolean(Method->isVariadic());
 Hash.AddBoolean(Method->isSynthesizedAccessorStub());
 Hash.AddBoolean(Method->isDefined());
-Hash.AddBoolean(Method->isOverriding());
 Hash.AddBoolean(Method->isDirectMethod());
 Hash.AddBoolean(Method->isThisDeclarationADesignatedInitializer());
 Hash.AddBoolean(Method->hasSkippedBody());
Index: clang/lib/AST/ODRDiagsEmitter.cpp
===
--- clang/lib/AST/ODRDiagsEmitter.cpp
+++ clang/lib/AST/ODRDiagsEmitter.cpp
@@ -2096,7 +2096,8 @@
   << FirstDecl->getSourceRange();
   Diag(SecondDecl->getLocation(),
diag::note_module_odr_violation_mismatch_decl_unknown)
-  << SecondModule << FirstDiffType << SecondDecl->getSourceRange();
+  << SecondModule.empty() << SecondModule << FirstDiffType
+  << SecondDecl->getSourceRange();
   return true;
 }
 


Index: clang/test/Modules/compare-objc-nonisolated-methods.m
===
--- /dev/null
+++ clang/test/Modules/compare-objc-nonisolated-methods.m
@@ -0,0 +1,54 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// Test that different values of `ObjCMethodDecl::isOverriding` in different modules
+// is not an error because it depends on the surrounding code and not on the method itself.
+// RUN: %clang_cc1 -fsyntax-only -verify -I%t/include -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache -fmodule-name=Override %t/test-overriding.m
+
+//--- include/Common.h
+@interface NSObject
+@end
+
+//--- include/Indirection.h
+#import 
+
+//--- include/module.modulemap
+module Common {
+  header "Common.h"
+  export *
+}
+module Indirection {
+  header "Indirection.h"
+  export *
+}
+module Override {
+  header "Override.h"
+  export *
+}
+
+//--- include/Override.h
+#import 
+@interface SubClass: NSObject
+- (void)potentialOverride;
+@end
+
+//--- Override_Internal.h
+#import 
+@interface NSObject(InternalCategory)
+- (void)potentialOverride;
+@end
+
+//--- test-overriding.m
+//expected-no-diagnostics
+// Get non-modular version of `SubClass`, so that `-[SubClass potentialOverride]`
+// is an override of a method in `InternalCategory`.
+#import "Override_Internal.h"
+#import 
+
+// Get modular version of `SubClass` where `-[SubClass potentialOverride]` is
+// not an override because module "Override" doesn't know about Override_Internal.h.
+#import 
+
+void triggerOverrideCheck(SubClass *sc) {
+  [sc potentialOverride];
+}
Index: clang/lib/AST/ODRHash.cpp
===
--- clang/lib/AST/ODRHash.cpp
+++ clang/lib