Re: Mac OS 9 (Lamp) port

From

JJJoshua Juran <jjuran@gmail.com>

Date

May 31, 2010, 04:35 UTC

Message-ID

<2776E3E5-234F-4A94-B06A-F4D83D87A414@gmail.com>

In-Reply-To

<20100531031906.GA465@progeny.tock>

On May 30, 2010, at 8:19 PM, Jonathan Nieder wrote:

> Joshua Juran wrote:
>
>> http://github.com/jjuran/git.git

Apparently <http://github.com/jjuran/git> works better.

> I assume the lamp-git branch is the interesting one.  Some of

> these patches (e.g., a3b378781) look like they might be of general

$ git show a3b378781
commit a3b378781228d59a13ee055d06c8f5465b543178
Author: Joshua Juran <jjuran@metamage.com>
Date:   Sun Nov 29 03:57:54 2009 -0800

     Avoid pointless use of one-celled array (which also confounds  
Metrowerks C).

     Pass the address of fetch_refspec_str directly to  
parse_refspec_internal()
     instead of copying fetch_refspec_str to an array and passing that.

     Metrowerks C won't initialize aggregates with values determined  
at runtime, and
     we can't use the C++ translator for parse_refspec_internal()  
because it uses the
     same identifier (refspec) for a variable as an existing struct  
type.

     That, and there's no purpose to the array in the first place.

diff --git a/remote.c b/remote.c
index c70181c..ade0424 100644
--- a/remote.c
+++ b/remote.c
@@ -657,10 +657,9 @@ static struct refspec *parse_refspec_internal 
(int nr_refspec, const char **refsp

  int valid_fetch_refspec(const char *fetch_refspec_str)
  {
-       const char *fetch_refspec[] = { fetch_refspec_str };
         struct refspec *refspec;

-       refspec = parse_refspec_internal(1, fetch_refspec, 1, 1);
+       refspec = parse_refspec_internal(1, &fetch_refspec_str, 1, 1);
         free_refspecs(refspec, 1);
         return !!refspec;
  }

Yes, this is the patch I suggested to Gary V. Vaughan.

> interest, while others (e.g., 6e6b106d) reveal Metrowerks to be
> a bit braindead.

$ git show 6e6b106d
commit 6e6b106dd6c561db726f90e2db07beeef1f716f9
Author: Joshua Juran <jjuran@metamage.com>
Date:   Wed Apr 8 01:51:04 2009 -0700

     Cast compare_info() arguments to (const struct pack_info *const *).

     Otherwise, Metrowerks C reports "illegal implicit conversion  
from 'const void *'
     to 'struct pack_info *const *'".

diff --git a/server-info.c b/server-info.c
index 4098ca2..a9a0188 100644
--- a/server-info.c
+++ b/server-info.c
@@ -132,8 +132,8 @@ static int read_pack_info_file(const char *infofile)

  static int compare_info(const void *a_, const void *b_)
  {
-       struct pack_info *const *a = a_;
-       struct pack_info *const *b = b_;
+       const struct pack_info *const *a = a_;
+       const struct pack_info *const *b = b_;

         if (0 <= (*a)->old_num && 0 <= (*b)->old_num)
                 /* Keep the order in the original */

I certainly won't defend Metrowerks C 2.4.1, but the patch is  
harmless enough.

> Have you thought about getting git to compile in C++ mode, where

If the Git maintainers are interested in this, I'm glad to do it.  I  
expect it to require generally less maintenance than my c++- 
initializer-fix branch, which is rather ugly to boot.

> Metrowerks might be a little more sane[1]?  Sure, this runs into

Well, I wasn't going to suggest general C++ compatibility just for my  
port, but if there's interest, then yes, that would obviate my (by  
far) largest and nastiest patch.

> basically all the major incompatibilities between C and C++[2], but
> that might not be insurmountable:

Writing code which is both valid C and valid C++ is not difficult.

>  . No implicit conversion from void* to other: don’t use void *,
>    then.  With type-safe interfaces like
>
> #define typed_malloc(size, type) (type *)xmalloc(size)
> #define malloc_many(nmemb, type) typed_malloc((nmemb) * sizeof 
> (type), type)
>
>    one can take advantage of type checking without the annoyance of
>    casts at the call site.

Replacing xmalloc(size) with typed_malloc(size, struct foo) instead  
of (struct foo *)xmalloc(size) still feels like a cast to me.  IMHO  
it adds no safety and reduces readability.

Maybe you could have:

	#define alloc_n(type, n)  ((type*)xmalloc(sizeof (type) * n))

to factor the multiplication out of the calling code.  But for a  
plain data buffer, nothing is shorter or clearer than a simple  
(char*) cast.

>    Another place git uses void * is for low-level access to the
>    object database, because it is not obvious whether objects data
>    should use char * or unsigned char *.  unsigned char * should
>    be fine.

If there were a configurable typedef defining this, I'd use char*  
since the Metrowerks debugger by default displays it as a C string  
(as opposed to a Pascal string for unsigned char* -- it's a Mac OS  
convention).  But that's a minor point and not one I'd spend time  
advocating.

>  . Use of C++ keywords:
>
> #ifdef __cplusplus
> #define template git_template
> #define typename git_typename
> #endif
>
>    It is not like this is an actual C++ program.

My c++initializer-fix patch does this locally in a few places.  I  
suppose it makes sense to nail them all at once in git-compat-util.h.

>  . Assignment of ints to enums is forbidden: okay, this one is
>    not worth working around.  Does Metrowerks have an option to turn
>    off this piece of C++ insanity?

It's a feature.  :-)  I use enums for type-safety in my C++ POSIX  
wrapper library.  p7::wait() returns p7::wait_t, and if you try to  
pass that to p7::exit() (which takes p7::exit_t) you get a compile  
error (instead of a program that 'works' when the wait status is zero).

(No, I'm not aware of an option to disable this restriction.)

If you want the type-safety, you have to pay for it.  Otherwise,  
declare variables and parameters as integral types instead.  I don't  
care one way or the other -- I just want git to work on Lamp, and if  
there's community interest in C++ compatibility then I can assist  
with that.

Josh

P.S.  Every patch in my git repository is Signed-off-by: Joshua Juran  
<jjuran@gmail.com> (or s/gmail/metamage/).