No subject

Ziemowit Laski zlaski@apple.com
Mon Aug 16 19:28:00 GMT 2004


Mark, Zack,

Please see my responses below.

> > This is by far the biggest chunk of the ObjC++ integration work; its
> > purpose is to set the stage
> > for ObjC++ proper.
> >
> > I've broken down the patch into two parts, A and B.  Part A (this
> > one) touches common parts of the compiler, and therefore requires
> > global approval.
>
> You need to explain, in detail, what each change does and why.
>
> You have broken up this patch along the wrong lines.  Never break a
> patch into pieces which could not be committed independently.  Changes

The reason I broke the patch up is simply to ease the review process, 
since
I can take maintainer responsibility for a large chunk of it.  I 
apologize if
this is confusing; this _is_ really all one giant patch.

> which are strictly mechanical - e.g. renaming functions - should
> always be submitted separately from other changes.  Prototypes for new
> functions should always be submitted alongside the new functions
> themselves.  In this case it probably makes sense for you to send a
> patch consisting solely of mechanical changes; a patch consisting
> solely of addition of new functions (it's okay to add new functions
> before the code that uses them); a patch consisting of miscellaneous
> changes to common code, *along with* the minimal set of associated
> changes to the Objective C front end; and finally the remainder of the
> changes to the Objective C front end.  You are expected to test,
> submit for approval, and (if approved) commit each of these patches
> independently.

This is the part that I find problematic. :-(  The work contained in the
two patches I posted last night, in addition to a couple of patches I
committed previously (and a few more I have yet to offer) is all part
of my ObjC++ integration (approved by the Steering Committee for 3.5
integration).  Furthermore, these bits already live in
objc-improvements-branch, available for the bootstrapping pleasure of 
all.

Therefore, I would kindly request that my ongoing work be treated as a
branch integration project.  Breaking this patch up into numerous
tiny pieces that when put together will reconstitute the original patch
does not offer any benefits that I can think of, and does in fact have
two major drawbacks: (1) it takes a lot more time and (2) it is more
susceptible to pilot error due to its fragmented nature.

Mark, it would be very helpful if you could opine as to how I should
proceed.  Of course, all comments and criticisms (such as those below)
are more than welcome, and I shall address them.  I'd just just that
I'd much prefer to (continue to) receive such comments regarding
the patches as I posted them. :-)

Now on to Zack's nits:

> > +/****** OBJECTIVE-C / OBJECTIVE-C++ ENTRY POINTS ******/
>
> No SCREAMING, no extra asterisks:
>
>   /* Objective-C and/or Objective-C++ entry points.  */

Ok; didn't mean to yell at anyone. :-)
>
> > +/* The following ObjC/ObjC++ functions are called by the C and/or 
> C++
> > +   front-ends; they all must have corresponding stubs in 
> stub-objc.c.
> > */
>
> Your mailer is still word-wrapping patches.  Fix it.

Hm... it looks fine from here; perhaps I shall explicitly attach patches
instead of pasting them into the window from now on. :-)

>
> > @@ -6662,6 +6664,12 @@ build_cdtor (int method_type, tree cdtor
> >  {
> >    tree body = 0;
> >
> > +  /* The Objective-C metadata initializer (if any) must be run
> > +     _before_ all other static constructors.  */
> > +  if (c_dialect_objc () && (method_type == 'I')
> > +      && objc_static_init_needed_p ())
> > +    cdtors = objc_generate_static_init_call (cdtors);
> > +
> >    if (!cdtors)
> >      return;
>
> This chunk of logic belongs in the caller of build_cdtor.  It can be
> expressed much more cleanly there.

Ok; I'll take a look.

> > -  if (!objc_is_public (datum, component))
> > +  if (c_dialect_objc () && !objc_is_public (datum, component))
>
> Most objc_* functions are designed not to need guarding with
> c_dialect_objc(), therefore I think it best if all of them don't; in
> other words, please cause objc_is_public to always return true when
> !c_dialect_objc, so this change will be unnecessary.

Again, perhaps Mark can issue a ruling here.  I thought that 
c_dialect_objc()
should be used because (1) it offers a clear demarcation point (i.e., 
"this is
ObjC-specific functionality") and (2) it improves performance (checking 
a bit
is a lot quicker than calling a function).

>
> > -  {".i", "@cpp-output", 0, 1, 0},
> > -  {"@cpp-output",
> > +  {".i", "@c-cpp-output", 0, 1, 0},
> > +  {"@c-cpp-output",
>
> This is a gratuitous change to the user interface which may break
> scripts.  You may not make this change.  (You may add -x c-cpp-output,
> but -x cpp-output must continue to have its existing meaning.)

Yes, you're right.  Truth be told, this is the one piece I actually
_should_ yank out my patch, since it is a logically separate animal.

Thanks,

--Zem
--------------------------------------------------------------
Ziemowit Laski                 1 Infinite Loop, MS 301-2K
Mac OS X Compiler Group        Cupertino, CA USA  95014-2083
Apple Computer, Inc.           +1.408.974.6229  Fax .5477



More information about the Gcc-patches mailing list