This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re:
- From: Ziemowit Laski <zlaski at apple dot com>
- To: Mark Mitchell <mark at codesourcery dot com>, Zack Weinberg <zack at codesourcery dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Mon, 16 Aug 2004 12:07:50 -0700
- Subject: Re:
- References: <E9D208E2-EFA8-11D8-8323-000393673036@apple.com>
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