llvm dev - Aug 2010 - [LLVMdev] MC-JIT Streamer 1/3

I was delayed creating the smaller patches, but finally I had some time to put
the first set together. There are three small patches, the first two are classes
the MCJITStreamer uses, and the last patch is the MCJITStreamer class itself.

- Jan

--- On Sun, 8/1/10, Daniel Dunbar <daniel at zuster.org> wrote:
> From: Daniel Dunbar <daniel at zuster.org>
> Subject: Re: [LLVMdev] MC-JIT Patches 2/3
> To: "Jan Sjodin" <jan_sjodin at yahoo.com>
> Cc: "LLVM Developers Mailing List" <llvmdev at cs.uiuc.edu>
> Date: Sunday, August 1, 2010, 6:58 PM
> Hi Jan,
> 
> I would rather not work with a patch this large. Can you
> pull out the
> addition of the MCJITStreamer into its own patch, and we
> can iterate
> on getting that in as a single commit? I realize it won't
> work or do
> anything useful, but I can't deal with reviewing patches
> this large.
> The main thing I am concerned about is getting the basic
> design of how
> the streamer and the assembler and the object writer should
> interact.
> 
> I also have a few general style comments:
>  1. Please use:
> --
> case A: {
>   ...
> }
> --
> instead of:
> --
> case A:
>   {
>     ...
>   }
> --
> 
>  2. Please use complete sentences in comments, including
> leading
> capitalization and punctuation. Also, please put a space
> after the
> comment marker ('/// Class', not '///class').
> 
>  3. Use doxygen comments when appropriate (for example, on
> methods,
> instance variables).
> 
> I have various comments on MCJITStreamer itself, but I will
> hold them
> until we have an individual patch for that class.
> 
>  - Daniel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0021_mcjit_rawostream.patch
Type: text/x-diff
Size: 5264 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20100820/91e4d881/attachment.patch>

Hi Jan,

Just a random comment, the indentation in mem_raw_ostream::write_impl
and mem_raw_ostream::current_pos should also be 2 spaces instead of 4!

On Fri, Aug 20, 2010 at 12:38 PM, Jan Sjodin <jan_sjodin at yahoo.com>
wrote:
> I was delayed creating the smaller patches, but finally I had some time to
put the first set together. There are three small patches, the first two are
classes the MCJITStreamer uses, and the last patch is the MCJITStreamer class
itself.
>
> - Jan
>
> --- On Sun, 8/1/10, Daniel Dunbar <daniel at zuster.org> wrote:
>
>> From: Daniel Dunbar <daniel at zuster.org>
>> Subject: Re: [LLVMdev] MC-JIT Patches 2/3
>> To: "Jan Sjodin" <jan_sjodin at yahoo.com>
>> Cc: "LLVM Developers Mailing List" <llvmdev at
cs.uiuc.edu>
>> Date: Sunday, August 1, 2010, 6:58 PM
>> Hi Jan,
>>
>> I would rather not work with a patch this large. Can you
>> pull out the
>> addition of the MCJITStreamer into its own patch, and we
>> can iterate
>> on getting that in as a single commit? I realize it won't
>> work or do
>> anything useful, but I can't deal with reviewing patches
>> this large.
>> The main thing I am concerned about is getting the basic
>> design of how
>> the streamer and the assembler and the object writer should
>> interact.
>>
>> I also have a few general style comments:
>>  1. Please use:
>> --
>> case A: {
>>   ...
>> }
>> --
>> instead of:
>> --
>> case A:
>>   {
>>     ...
>>   }
>> --
>>
>>  2. Please use complete sentences in comments, including
>> leading
>> capitalization and punctuation. Also, please put a space
>> after the
>> comment marker ('/// Class', not '///class').
>>
>>  3. Use doxygen comments when appropriate (for example, on
>> methods,
>> instance variables).
>>
>> I have various comments on MCJITStreamer itself, but I will
>> hold them
>> until we have an individual patch for that class.
>>
>>  - Daniel
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>


-- 
Bruno Cardoso Lopes
http://www.brunocardoso.cc