[Mono-dev] [PATCH] mono debugger for Mac OS X

[jonas echterhoff]

On Apr 21, 2009, at 10:06 PM, Miguel de Icaza wrote:

> Hello Jonas,
>
>    Some feedback on the patches
>
> mono.diff:
>
> 	* In general, they are missing ChangeLog entries that document
> 	  what the changes are.   In particular it would be useful
> 	  to document the rationale behind the changes in the signal
> 	  definitions in the libgc directory.
>
> 	* The coding style should adhere to the Mono coding style,
>  	  for example isInited should become "is_initialized", for
> 	  more info:
>
> 		http://mono-project.com/Coding_Guidelines
>
> 	  It is not clear why that change was needed in the first
> 	  place, so a ChangeLog entry here would help.   Since we
> 	  have not ran into this in the past, I suspect that this
> 	  might be hiding a problem elsewhere.
>
> 	* The change to mono/mini/Makefile.am probably should be
> 	  a platform_ldflags and be set in configure.in instead
> 	  to avoid yet another if macro there

I hope to get multi-thread support to work properly this week - then I  
will submit a new patch. I will take care to add proper ChangeLog  
entries, and address these issues.

> 	* The tests/tests-config change, why is that necessary?
> 	  The proper change would be to use os specifiers there
> 	  see the mono-config(5) man page for details.

It is not. The file gets automatically changed/generated on make, and  
thus was accidentally added to the .diff. Does it have to be in svn at  
all?

> debugger.diff:
>
> 	* The reason why the configure.in test for PLATFORM_X86_DARWIN
> 	  does not work is that this is an autoconf macro that is
> 	  only exposed to the automake world (the Makefile.am) and it
> 	  is not available during configure time.
>
> 	  Inside configure.ac or  configure.in you must use the shell-
> 	  level variables that you defined (the ones that actually
> 	  detected PLATFORM_x86_DARWIN).

you mean this?

+#not sure why, but I cannot get this test to pass on OS X.
+if test x$PLATFORM_X86_DARWIN = "xno" ; then

But the test for PLATFORM_X86_DARWIN actually works. What does not  
work is the checking code in between the if, which is why I had to add  
that test.

jonas