[PATCH] D154459: [ODRHash] Stop hashing `ObjCMethodDecl::isOverriding` as it doesn't capture inherent method quality.
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.
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.
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