Hi, comments inline: On Mon, Aug 2, 2010 at 5:00 AM, Zoltan Varga <var...@gmail.com> wrote:
> Hi, > > Some further comments: > > - it would be nice to follow the mono coding conventions for the changes, > i.e. > put a space before '(' in calls, use tabs in files where tabs are used > for indentation, etc. > Yes, we've been trying to do that for future modifications in amd64. I can address these issues with x86 with an alternate patch if you'd like, let me know. > - some of the #ifdefs could be avoided by adding a NACL_SIZE() macro: > > #if defined(__native_client_codegen__) || defined(__native_client__) > #define NACL_SIZE(a, b) (b) > #else > #define NACL_SIZE(a, b) (a) > > and using it for things like the trampoline buffer sizes: > > +#ifdef __native_client_codegen__ > + tramp_size = 128; > +#else > tramp_size = 64; > +#endif > > would become: > > tramp_size = NACL_SIZE (64, 128); > > It looks like you're trying to optimize for as few preprocessor defines as possible. If that's the case, we'll try to carry that sentiment over to our new changes too. One thing we've been talking about locally is what to do in amd64 (where we're having to make more changes than in x86) when we have divergent behavior between normal mono and NaCl. Trying to be good guests in the Mono codebase, we started putting the mono codeblocks first, like so: #if defined(__default_codegen__) //normal mono behavior #elif defined(__native_client_codegen__) //new nacl behavior #else (if needed) //optional default case #endif And then defining __default_codegen__ for non-nacl builds. Obviously this is a little messy as it adds a new define for every other build, but we thought it made it clear that it is the default path, rather than using "#ifndef __native_client_codegen__". I find that it's easier to parse code with a lot of ifdefs rather than a mixture of ifndefs and ifdefs. Does anyone have any strong opinions one way or another about how we do this? Zoltan > > > On Fri, Jul 16, 2010 at 1:30 AM, Elijah Taylor <elijahtay...@google.com>wrote: > >> Hi, here's an updated patch with your feedback addressed. I re-based the >> diff closer to head revision (r160382) to include the other changes of ours >> that already landed, as well as make sure we're still compatible with >> current Mono development. >> >> In general this diff should have a smaller impact on the .c files: >> mini-x86.c, exceptions-x86.c, tramp-x86.c specifically, and the Native >> Client changes are a little more grouped together rather than spread out. >> >> A couple of points separate from the feedback: >> 1) I fixed a bug in my implementation of genmdesc.pl changes, so that >> will be different from the previous patch >> 2) There's a small typo at head revision in mono/mini/tramp-x86.c which >> says "rethow" instead of "reth*r*ow" for your rethrow exception >> trampoline. This is also fixed in my patch. >> >> As always feedback is appreciated from everyone. >> >> >> -Elijah >> >> >> On Tue, Jul 6, 2010 at 6:35 AM, Zoltan Varga <var...@gmail.com> wrote: >> >>> Hi, >>> >>> >>>> One possibility is to pad out all x86_prefix instructions to the nearest >>>> 32-byte boundary, but that could really bloat things depending on how often >>>> they're used. Do you have any idea of the prefix to non-prefix instruction >>>> ratio? It seems like it'd be pretty low based on looking at the code but I >>>> haven't looked at any actual metrics. >>>> >>>> >>> I think that would be ok, they are seldom used. >>> >>> Zoltan >>> >> >> >
_______________________________________________ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list