stephanemoore marked 2 inline comments as done.
stephanemoore added a comment.
Thanks for the review!
================
Comment at: clang-tools-extra/clang-tidy/objc/MissingHashCheck.cpp:56
+ const auto *ID = Result.Nodes.getNodeAs<ObjCImplementationDecl>("impl");
+ diag(ID->getLocation(), "%0 implements -isEqual: without implementing -hash")
+ << ID;
----------------
aaron.ballman wrote:
> stephanemoore wrote:
> > aaron.ballman wrote:
> > > Do you think we could generate a fixit to add the `hash` method? Do you
> > > think we could even add a default implementation that returns the pointer
> > > to the object (assuming that's the correct default behavior)?
> > > Do you think we could generate a fixit to add the hash method?
> >
> > I think it would be pretty tough to generate a reasonable hash method
> > without knowing the equality and hashing semantics that the scenario calls
> > for.
> >
> > Here is an analogous situation presented in C++ (please excuse the hastily
> > assembled sample code):
> > ```
> > namespace {
> >
> > class NSObject {
> > public:
> > NSObject() {}
> > virtual ~NSObject() {}
> >
> > virtual bool isEqual(const NSObject *other) const {
> > return this == other;
> > }
> > virtual unsigned long long hash() const {
> > return (unsigned long long)this;
> > }
> > };
> >
> > }
> >
> > #include <stdio.h>
> > #include <string>
> >
> > namespace {
> >
> > class Movie : public virtual NSObject {
> > private:
> > std::string name;
> > std::string language;
> >
> > public:
> > Movie(std::string name, std::string language) : name(name),
> > language(language) {}
> > ~Movie() override {}
> > bool isEqual(const NSObject *other) const override {
> > if (auto otherMovie = dynamic_cast<const Movie *>(other)) {
> > // Movies with the same name are considered equal
> > // regardless of the language of the screening.
> > return name == otherMovie->name;
> > }
> > return false;
> > }
> > unsigned long long hash() const override {
> > return name.length();
> > }
> > };
> >
> > }
> > ```
> >
> > As before, the base class uses pointer equality and the pointer as a hash.
> > A subclass may arbitrarily add additional state but only the developer
> > knows which added state factors into equality operations and consequently
> > should be considered—but not necessarily required—in the hash operation.
> > The matter can technically get even more complicated if an object stores
> > state externally. I would hope that externally stored state would not
> > factor into the equality operation of an object but I hesitate to make an
> > assumption.
> >
> > The developer is also in the best position to prioritize different
> > properties of the hash function including performance, collision
> > resistance, uniformity, and non-invertibility.
> >
> > Writing effective hash functions is probably difficult independent of the
> > programming language but it might help to consider some specific examples
> > in Objective-C.
> > [GPBMessage](https://github.com/protocolbuffers/protobuf/blob/ffa6bfc/objectivec/GPBMessage.m),
> > the Objective-C base class for Google Protocol Buffer message classes,
> > implements `-hash` but has an [extensive
> > comment](https://github.com/protocolbuffers/protobuf/blob/ffa6bfc/objectivec/GPBMessage.m#L2749)
> > explaining that its complex but generic implementation is not generally
> > optimal and recommends that developers override `-hash` and `-isEqual:` to
> > optimize for runtime performance. In contrast, the basic collection classes
> > in Apple's Foundation framework have [surprisingly simple hash
> > behavior](https://github.com/stephanemoore/archives/blob/master/objc/tips/hashing-basic-collections.md)
> > that clearly indicate priority to runtime performance over uniformity and
> > collision resistance. The former is a conservatively expensive hash
> > function and the latter is a conservatively inexpensive hash function.
> >
> > > Do you think we could even add a default implementation that returns the
> > > pointer to the object (assuming that's the correct default behavior)?
> >
> > A hash returning the object pointer is already inherited from the
> > superclass (i..e, `-[NSObject hash]`). Defining an override that returns
> > the object pointer would be a functional no-op for classes directly derived
> > from `NSObject` (although the explicit override could be useful as a signal
> > of intended behavior).
> > A hash returning the object pointer is already inherited from the
> > superclass (i..e, -[NSObject hash]). Defining an override that returns the
> > object pointer would be a functional no-op for classes directly derived
> > from NSObject (although the explicit override could be useful as a signal
> > of intended behavior).
>
> Ah, my ObjC knowledge is weak and I was thinking that the one inherited from
> `NSObject` would be hidden. Thank you for the detailed explanation, that
> makes a lot of sense to me.
> Thank you for the detailed explanation, that makes a lot of sense to me.
My pleasure!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67737/new/
https://reviews.llvm.org/D67737
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
