[llvm-commits] [llvm] r128415 - in /llvm/trunk: include/llvm-c/Disassembler.h lib/MC/MCDisassembler/Disassembler.cpp lib/MC/MCDisassembler/Disassembler.h

[Chris Lattner]

On Mar 28, 2011, at 11:25 AM, Kevin Enderby wrote:

> Author: enderby
> Date: Mon Mar 28 13:25:07 2011
> New Revision: 128415
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=128415&view=rev
> Log:
> Again adding a C API to the disassembler for use by such tools as Darwin's
> otool(1), this time with the needed fix for case sensitive file systems :) .
> This is a work in progress as the interface for producing symbolic operands is
> not done.  But a hacked prototype using information from the object file's
> relocation entiries and replacing immediate operands with MCExpr's has been
> shown to work with no changes to the instrucion printer.  These APIs will be
> moved into a dynamic library at some point.

Nice!  Sorry for the delay reviewing this, a few comments:

> +typedef int (*LLVMOpInfoCallback)(void *DisInfo,
> +                                  uint64_t PC,
> +                                  uint64_t Offset,
> +                                  uint64_t Size,
> +                                  int TagType,
> +                                  void *TagBuf);

If I understand things correctly, TagType only has a couple of possible values.  Instead of passing it around as an 'int', please define the possible values using an enum.

> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* !defined(__cplusplus) */

Traditionally, this is put at the top of the file, after the #includes.  Please move it up there.

> /**
>  * The operand VariantKinds for symbolic disassembly.
>  */
> #define LLVMDisassembler_VariantKind_None 0 /* all targets */
> 
> /**
>  * The ARM target VariantKinds.
>  */
> #define LLVMDisassembler_VariantKind_ARM_HI16 1 /* :upper16: */
> #define LLVMDisassembler_VariantKind_ARM_LO16 2 /* :lower16: */

Can these be enums instead of #defines?  Enums have a number of advantages, including enforced types and that they show up in the debugger (even for C code).

-Chris