Re: [objc-improvements] Warnings when compiling GNUStep

[Alexander Malmberg]

Ziemowit Laski wrote:
> The objc-improvements-branch compiler can now successfully churn its
> way through the GNUstep compile-all script, although it does produce
> some new warning messages along the way, and I wanted to solicit your
> opinion as to what (if anything) to do about it.

I few days ago, I sent logs of warnings of -gui and -base in GNUstep to
the lists, but compiled with my patch that cleans up the behavior and
warnings a bit more. Thus, some of the issues are already fixed in my
patch.

> First and foremost, we have numerous instances of the following:
> 
>    GSObjCRuntime.m: In function `GSAutoreleasedBuffer':
> 
>    GSObjCRuntime.m:1497: warning: `Class' may not respond to
> `+methodForSelector:'
[snip]
> (The "incompatible pointer type" warning is a direct consequence of not
> finding
> a suitable method in the first place.)  There does exist an instance
> `-methodForSelector:'
> in the 'NSObject' class (which probably gets used in this case),

Yes, this is what happens when the code runs.

> but
> since Objective-C
> allows multiple root classes, the compiler cannot statically decide to
> focus on NSObject.
> What should we do?  I see 3 choices:
>    (1) Leave things as they are.  Users will be able to suppress the
> above warning by
>        casting the receiver to the root class (in above case, '(NSObject
> *)').
>    (2) Build a list of all plausible root classes (i.e., classes without
> superclasses),
>        and consult all instance methods for those classes when
> dispatching messages to
>        'Class'
>    (3) Treat messages to 'Class' the same way we treat messages to 'id',
> i.e., consult
>        all class and instance methods available.  This is what the
> compiler appears to
>        have done until now (in its mercurial and buggy way).

I've implemented 2. in my patch, and I think this is correct and
consistent with the intention of the current behavior. I also have some
test cases for this. The only issue is instance methods in protocols
conformed to by root classes. My patch does not include them in class
method hash list. Arguably, it should, but implementing that is tricky,
and I don't think it's going to cause any problems (at worst, the root
classes will just have to repeat the prototypes in their @interfaces;
since there are very few root classes, I don't think that fixing this
will be a problem).

> Personally, I like types (and compiler's warnings about them) :-), so
> I'd advocate we stick
> with (1), but we could also do (3).  I'd really, really hate to have to
> implement (2), however.
> Not only is it extra work, but it will adversely affect compile time.

Correctness and sane behavior is, at least IMHO, much more important
than compile time. Besides, what I do is add instance methods of root
classes to the global class method hash list. The performance impact for
this should be negligible.

(Some of my other changes might affect performance a bit. As I said, I
think good behavior is more important. If performance really does drop,
I could try to optimize some parts of it. Also, any claims of speed loss
will have to be backed up with benchmarks that show that there actually
is a significant loss of speed; I suspect that extra time used in the
objective-c frontend is dwarfed by time spent in optimization passes. I
may do some benchmarking later to see if this is true or not.)

[snip]
> they match up).  However, in a few cases we're dealing with the idiom
> 
>     [[ClassName alloc] init...]
> 
> in which case one _could_ make the argument that any method named
> 'alloc' should be treated
> specially by the compiler, i.e., as having a return type covariant with
> the receiver.  What
> do you think?

This was suggested in discuss-gnustep, too. I don't think this is
appropriate. The fact that +alloc does this is a convention in certain
libraries built on top of the objective-c language. Tying the behavior
of the language to any specific library (or libraries) built on the
language is wrong.

The warning in this case is just caused by a breach of convention by
NSDistributedLock. The convention (in anticipation of these changes, or
to satisfy earlier and more pedantic compilers?) in GNUstep is that all
-init... methods return 'id'. The prototypes that didn't follow this
(all two of them) will be fixed.

We might want to look at adding "magic types" or special attributes so
that libraries can tell the compiler about things like this, but I'm not
convinced there's an acceptable solution there, or that it's necessary
at all. Besides, I want to get these other changes wrapped up first. :)

[snip]
> all of which do seem reasonable to me, but please double-check.

We've looked at and discussed my list of warnings (which includes more
warnings since my patch checks things more carefully). The only warnings
that have caused any discussion are:

* A conflict between different -compare:s. I believe David Ayers cc:d
you about this. Basically, it's a real conflict that needs to be fixed
in the libraries. The compiler (with my patch) is doing the right thing.

* Extra warnings at +alloc caused by a breach of convention by a class.
The class has been fixed; the compiler is doing the right thing.

* The -validateMenuItem: conflict. This is case c. in my list, and I
think it's the only remaining issue. We need to decide whether to use
the fallback prototype, try to merge, or pick one randomly. I have
implemented fallback, but changing back to random is easy. I think
merging could be implemented fairly quickly if we decide to do that.

Either way, I'll send the patch and all the test cases we have so far to
gcc-patches tonight. I doubt that case c. won't be decided by then, but
at least you and others can look at the other parts. :)

- Alexander Malmberg