[llvm-commits] [llvm] r75027 - in /llvm/trunk: include/llvm/CodeGen/ include/llvm/Support/ lib/Target/Alpha/ lib/Target/Alpha/AsmPrinter/ lib/Target/IA64/ lib/Target/IA64/AsmPrinter/ lib/Target/MSP430/ lib/Target/Mips/AsmPrinter/ lib/Target/PIC16/ lib/Target/Sparc/AsmPrinter/ lib/Target/X86/ lib/Target/XCore/ utils/TableGen/

[Török Edwin]

On 2009-07-08 23:52, Chris Lattner wrote:
> On Jul 8, 2009, at 1:46 PM, Török Edwin wrote:
>   
>> Thanks for reviewing my commit, looks like I got it wrong in many  
>> places.
>> At least error handling should be more consistent now, asserts that  
>> are
>> meant to be assert are ;)
>>     
>
> Well I only listed the ones I disagreed with, you got many more right :)
>
>   
>>>> +++ llvm/trunk/include/llvm/CodeGen/MachineCodeEmitter.h Wed Jul  8
>>>> 14:04:27 2009
>>>> @@ -18,6 +18,8 @@
>>>> #define LLVM_CODEGEN_MACHINECODEEMITTER_H
>>>>
>>>> #include "llvm/Support/DataTypes.h"
>>>> +#include "llvm/Support/ErrorHandling.h"
>>>> +#include "llvm/Support/raw_ostream.h"
>>>>
>>>>         
>>> Please don't include headers in other headers unless you cannot avoid
>>> it.
>>>
>>>
>>>       
>> I included it because then I don't need to include it in all the
>> *ISelDAGToDAG.cpp files.
>> I removed it now, and included the necessary headers in each .cpp  
>> file.
>>     
>
> Why do they need raw_ostream?
>   

Because of the TableGen change to report and errormessage for a not
handled instruction/opcode.
All the .inc files use raw_ostream.

>   
>>>>    case GlobalValue::DLLExportLinkage:
>>>> -      cerr << "DLLExport linkage is not supported by this target! 
>>>> \n";
>>>> -      abort();
>>>> +      llvm_report_error("DLLExport linkage is not supported by this
>>>> target!");
>>>>
>>>>         
>>> assertions. :)
>>>
>>>       
>> Thanks, I changed them. Quite a lot of copy+pasting between the  
>> targets,
>> isn't it?
>>     
>
> Yes, the asmprinters are copy and paste hell, I'm trying to help a bit  
> with the mc stuff.
>   

:)

>>> Thanks Edwin, most of this looks great.  Several of these below are
>>> assertions that have a message: the preferred style is something  
>>> like:
>>>
>>> #ifndef NDEBUG
>>>   cerr << "WHATEVER\n";
>>> #endif
>>>   unreachable();
>>>
>>>       
>> Ok, I changed LLVM_UNREACHABLE to do that.
>> I also used this style when a more complicated message was printed.
>>     
>
> Sounds good.
>
> Thanks Edwin,
>
>   

Thanks,
--Edwin