[cfe-commits] r137055 - in /cfe/trunk: include/clang/Basic/IdentifierTable.h include/clang/Sema/Sema.h lib/Sema/SemaDeclObjC.cpp test/SemaObjC/class-protocol-method-match.m test/SemaObjC/qualified-protocol-method-conflicts.m
Douglas Gregor
dgregor at apple.com
Mon Aug 8 11:34:31 PDT 2011
On Aug 8, 2011, at 11:03 AM, Fariborz Jahanian wrote:
> Author: fjahanian
> Date: Mon Aug 8 13:03:17 2011
> New Revision: 137055
>
> URL: http://llvm.org/viewvc/llvm-project?rev=137055&view=rev
> Log:
> objective-c: diagnose protocol inconsistencies in following
> situation. When a class explicitly or implicitly (through inheritance)
> "conformsTo" two protocols which conflict (have methods which conflict).
> This patch fixes the previous patch where warnings were coming out in
> non-deterministic order. This is 2nd part of // rdar://6191214.
A few comments below…
> Added:
> cfe/trunk/test/SemaObjC/qualified-protocol-method-conflicts.m
> - copied, changed from r136946, cfe/trunk/test/SemaObjC/qualified-protocol-method-conflicts.m
> Modified:
> cfe/trunk/include/clang/Basic/IdentifierTable.h
> cfe/trunk/include/clang/Sema/Sema.h
> cfe/trunk/lib/Sema/SemaDeclObjC.cpp
> cfe/trunk/test/SemaObjC/class-protocol-method-match.m
>
> Modified: cfe/trunk/include/clang/Basic/IdentifierTable.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/IdentifierTable.h?rev=137055&r1=137054&r2=137055&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/IdentifierTable.h (original)
> +++ cfe/trunk/include/clang/Basic/IdentifierTable.h Mon Aug 8 13:03:17 2011
> @@ -557,6 +557,11 @@
> bool operator!=(Selector RHS) const {
> return InfoPtr != RHS.InfoPtr;
> }
> +
> + bool operator < (Selector RHS) const {
> + return InfoPtr < RHS.InfoPtr;
> + }
> +
> void *getAsOpaquePtr() const {
> return reinterpret_cast<void*>(InfoPtr);
> }
Please don't add this; it doesn't work the way you expect. To compare source locations, use SourceManager::isBeforeInTranslationUnit().
> Modified: cfe/trunk/include/clang/Sema/Sema.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=137055&r1=137054&r2=137055&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Sema/Sema.h (original)
> +++ cfe/trunk/include/clang/Sema/Sema.h Mon Aug 8 13:03:17 2011
> @@ -493,6 +493,39 @@
> /// a potentially evaluated expression.
> typedef SmallVector<std::pair<SourceLocation, Decl *>, 10>
> PotentiallyReferencedDecls;
> +
> + // FIXME. Improve on accessibility.
> + class PROTOCOL_METHODS {
Please follow LLVM naming conventions.
> + public:
> + Selector Sel;
> + ObjCMethodDecl *Method;
> + PROTOCOL_METHODS(Selector S, ObjCMethodDecl *M)
> + : Sel(S), Method(M) {}
> + // Allow sorting based on selector's opaque pointer.
> + bool operator<(const PROTOCOL_METHODS &b) const {
> + return Sel < b.Sel;
> + }
> + };
> +
> + /// \brief The set of protocols declared in protocols qualifying a
> + /// class.
> + typedef SmallVector<PROTOCOL_METHODS, 16> MethodsInProtocols;
> +
> + class IDENTICAL_SELECTOR_METHODS {
> + public:
> + SourceLocation Loc;
> + ObjCMethodDecl *Method;
> + IDENTICAL_SELECTOR_METHODS(SourceLocation L, ObjCMethodDecl *M)
> + : Loc(L), Method(M) {}
> + // Allow sorting based on selector's source location.
> + bool operator<(const IDENTICAL_SELECTOR_METHODS &i) const {
> + return !(Loc < i.Loc);
> + }
> + };
> +
> + /// \brief Methods with identical selectors to be type-matched against
> + /// one another.
> + typedef SmallVector<IDENTICAL_SELECTOR_METHODS, 8> IdenticalSelectorMethods;
>
> /// \brief A set of diagnostics that may be emitted.
> typedef SmallVector<std::pair<SourceLocation, PartialDiagnostic>, 10>
> @@ -1780,7 +1813,12 @@
> void WarnExactTypedMethods(ObjCMethodDecl *Method,
> ObjCMethodDecl *MethodDecl,
> bool IsProtocolMethodDecl);
> -
> +
> + /// WarnOnMismatchedProtocolMethods - Issues warning on type mismatched
> + /// protocols methods and then returns true(matched), or false(mismatched).
> + bool WarnOnMismatchedProtocolMethods(ObjCMethodDecl *Method,
> + ObjCMethodDecl *MethodDecl);
> +
> bool isPropertyReadonly(ObjCPropertyDecl *PropertyDecl,
> ObjCInterfaceDecl *IDecl);
>
> @@ -1904,10 +1942,16 @@
> bool ImmediateClass,
> bool WarnExactMatch=false);
>
> + /// MatchIdenticalSelectorsInProtocols - Check that mathods with
> + /// identical selectors in all protocols of this class type match.
> + /// Issue warning if they don't.
> + void MatchIdenticalSelectorsInProtocols(const ObjCInterfaceDecl *CDecl);
> +
> /// MatchMethodsInClassAndItsProtocol - Check that any redeclaration of
> /// method in protocol in its qualified class match in their type and
> /// issue warnings otherwise.
> void MatchMethodsInClassAndItsProtocol(const ObjCInterfaceDecl *CDecl);
> +
>
> /// CheckCategoryVsClassMethodMatches - Checks that methods implemented in
> /// category matches with those implemented in its primary class and
>
> Modified: cfe/trunk/lib/Sema/SemaDeclObjC.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclObjC.cpp?rev=137055&r1=137054&r2=137055&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaDeclObjC.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDeclObjC.cpp Mon Aug 8 13:03:17 2011
> @@ -1281,6 +1281,35 @@
> }
> }
>
> +/// WarnOnMismatchedProtocolMethods - Issues warning on type mismatched
> +/// protocols methods and then returns true(matched), or false(mismatched).
> +bool Sema::WarnOnMismatchedProtocolMethods(ObjCMethodDecl *ImpMethodDecl,
> + ObjCMethodDecl *MethodDecl) {
> +
> + bool match = CheckMethodOverrideReturn(*this, ImpMethodDecl, MethodDecl,
> + true,
> + true, true);
> + if (!match)
> + return false;
> +
> + for (ObjCMethodDecl::param_iterator IM = ImpMethodDecl->param_begin(),
> + IF = MethodDecl->param_begin(), EM = ImpMethodDecl->param_end();
> + IM != EM; ++IM, ++IF) {
> + match = CheckMethodOverrideParam(*this, ImpMethodDecl, MethodDecl, *IM, *IF,
> + true, true, true);
> + if (!match)
> + return false;
> + }
> +
> + if (ImpMethodDecl->isVariadic() != MethodDecl->isVariadic()) {
> + Diag(ImpMethodDecl->getLocation(), diag::warn_conflicting_variadic)
> + << true;
> + Diag(MethodDecl->getLocation(), diag::note_previous_declaration);
> + return false;
> + }
> + return true;
> +}
> +
> /// WarnExactTypedMethods - This routine issues a warning if method
> /// implementation declaration matches exactly that of its declaration.
> void Sema::WarnExactTypedMethods(ObjCMethodDecl *ImpMethodDecl,
> @@ -1568,6 +1597,41 @@
> }
> }
>
> +/// CollectAllMethodsInProtocols - Helper routine to collect all methods
> +/// declared in given class's immediate and nested protocols.
> +static void CollectAllMethodsInProtocols(const ObjCContainerDecl *ContDecl,
> + Sema::MethodsInProtocols &InstMethodsInProtocols,
> + Sema::MethodsInProtocols & ClsMethodsInProtocols) {
> + if (const ObjCInterfaceDecl *CDecl = dyn_cast<ObjCInterfaceDecl>(ContDecl)) {
> + for (ObjCInterfaceDecl::all_protocol_iterator
> + PI = CDecl->all_referenced_protocol_begin(),
> + E = CDecl->all_referenced_protocol_end(); PI != E; ++PI)
> + CollectAllMethodsInProtocols(*PI, InstMethodsInProtocols,
> + ClsMethodsInProtocols);
> + }
> +
> + if (const ObjCProtocolDecl *PDecl = dyn_cast<ObjCProtocolDecl>(ContDecl)) {
> + for (ObjCProtocolDecl::instmeth_iterator I = PDecl->instmeth_begin(),
> + E = PDecl->instmeth_end(); I != E; ++I) {
> + ObjCMethodDecl *method = *I;
> + InstMethodsInProtocols.push_back(Sema::PROTOCOL_METHODS(method->getSelector(),
> + method));
> + }
> + for (ObjCProtocolDecl::classmeth_iterator I = PDecl->classmeth_begin(),
> + E = PDecl->classmeth_end(); I != E; ++I) {
> + ObjCMethodDecl *method = *I;
> + ClsMethodsInProtocols.push_back(Sema::PROTOCOL_METHODS(method->getSelector(),
> + method));
> + }
> +
> + for (ObjCProtocolDecl::protocol_iterator
> + PI = PDecl->protocol_begin(),
> + E = PDecl->protocol_end(); PI != E; ++PI)
> + CollectAllMethodsInProtocols(*PI, InstMethodsInProtocols,
> + ClsMethodsInProtocols);
> + }
> +}
We have a ton of these "find all of the methods in a protocol hierarchy", and I'm very concerned that they differ in semantics. Is there another you could re-use? Or perhaps provide a general visitor, and switch this (and other) callers over to that visitor?
> /// CollectMethodsInProtocols - This routine collects all methods declared
> /// in class's list and nested qualified protocols. Instance methods and
> /// class methods have separate containers as they have identical selectors.
> @@ -1612,6 +1676,77 @@
> ClsMethodsInProtocols);
> }
>
> +/// MatchMethodsWithIdenticalSelectors - Helper routine to go through list
> +/// of identical selector lists and issue warning for any type mismatche
> +/// of these methods.
> +static bool MatchMethodsWithIdenticalSelectors(Sema &S,
> + const Sema::MethodsInProtocols Methods) {
> + bool res = true;
> + int size = Methods.size();
> + int i = 0;
> + while (i < size) {
> + int upper = i;
> + while (upper < size &&
> + (Methods[i].Sel == Methods[upper].Sel))
> + upper++;
> + if (upper > i) {
> + int lo = i;
> + int hi = upper - 1;
> +
> + if (lo < hi) {
> + Sema::IdenticalSelectorMethods SelectedMethods;
> + for (int l = lo; l <= hi; l++) {
> + ObjCMethodDecl *method = Methods[l].Method;
> + Sema::IDENTICAL_SELECTOR_METHODS
> + SelectedMethod(method->getLocation(), method);
> + SelectedMethods.push_back(SelectedMethod);
> + }
> + llvm::array_pod_sort(SelectedMethods.begin(), SelectedMethods.end());
> + lo = 0; hi = SelectedMethods.size()-1;
> + while (lo < hi) {
> + ObjCMethodDecl *targetMethod = SelectedMethods[lo].Method;
> + for (int j = lo+1; j <= hi; j++) {
> + // match two methods;
> + ObjCMethodDecl *otherMethod = SelectedMethods[j].Method;
> + if (!S.WarnOnMismatchedProtocolMethods(targetMethod, otherMethod))
> + res = false;
> + }
> + ++lo;
> + }
> + }
> + }
> + i += upper;
> + }
> + return res;
> +}
This is really convoluted. I see why you're doing it---to walk over a selector at a time, and check all of the protocol methods that have that selector---but it would be cleaner with a different data structure. For example, a map from selector -> protocol methods with that selector. Whenever we get more than one protocol method with the selector, we add that selector to a queue that we visit later for checking. The resulting algorithm is amortized linear time rather than the O(n log n) you have now, and will be far simpler to implement.
Backing up a little bit, I think that both of my concerns about could easily by addressed by folding this checking into CheckObjCMethodOverride (expanding it if needed). We're already paying for a walk of the protocols, interfaces, etc., when we perform that operation, and it should give the consistency checking that we want. Was it not possible to use CheckObjCMethodOverride?
> +/// MatchIdenticalSelectorsInProtocols - Main routine to go through list of
> +/// class's protocols (and their protocols) and make sure that methods
> +/// type match across all protocols and issue warnings if they don't.
> +/// FIXME. This may move to static analyzer if performance is proven
> +/// prohibitive.
Have you done these performance comparisons?
- Doug
More information about the cfe-commits
mailing list