[llvm-commits] [llvm] r170288 - in /llvm/trunk/lib/Target/XCore: CMakeLists.txt InstPrinter/ InstPrinter/CMakeLists.txt InstPrinter/LLVMBuild.txt InstPrinter/Makefile InstPrinter/XCoreInstPrinter.cpp InstPrinter/XCoreInstPrinter.h LLVMBuild.txt MCTargetDesc/LLVMBuild.txt MCTargetDesc/XCoreMCTargetDesc.cpp Makefile XCore.td XCoreAsmPrinter.cpp XCoreMCInstLower.cpp XCoreMCInstLower.h

[Richard Osborne]

Thanks for you comments - I've fixed the issues you raised in 
r170319-r170321

On 16/12/12 19:32, Sean Silva wrote:
> Good to see XCore getting some love!
>
> +#ifndef XCOREMCINSTLOWER_H
> +#define XCOREMCINSTLOWER_H
> +#include "llvm/ADT/SmallVector.h"
>
> Why do you need SmallVector.h in this header?
Fixed in r170319
>
> +/// XCoreMCInstLower - This class is used to lower an MachineInstr into an
> +//                     MCInst.
>
> New code should follow the recommended doxygen style:
> <http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments>
Fixed in r170320
>
> +  } else if (!(SRE = dyn_cast<MCSymbolRefExpr>(Expr))) {
> +    assert(false && "Unexpected MCExpr type.");
>
> You can probably use isa<> instead of dyn_cast<> here. Also use
> llvm_unreachable instead of "assert(false)".
SRE is used below so the dyn_cast<> is necessary. I simplified this code 
in r170321, hopefully it is now easier to read.