[llvm-commits] [PATCH] ULEB FIXME in ARM back-end

[Renato Golin]

On 9 August 2011 04:28, Jason Kim <jasonwkim at google.com> wrote:
> Patch looks okay ---

Hi Jason,

Thanks for reviewing the patch. I'm assuming there is no immediate
problem in duplicating the getULEBSize() here and that the
AttributeItem structure is fine where it is.


> Just a couple of nits. Please fix and commit.
> 1. no need to typedef a anon struct AttributeItem - this is C++ (enter
> appropriate "300" joke here)

Working with a mix of C and C++ has its tolls... ;) I've declared the
structure as AttributeItemType rather than leaving it unnamed. I need
this to use it in smallVector template argument.


> 2. Are you sure there is not a better way to null terminate the string
> then +          Streamer.EmitIntValue(0, 1); // '\0'

It's used all over the AsmPrinter to print null-terminated strings
(see Finish()). Besides, MCStreamer doesn't have an EmitNullString()
or equivalent from StringRef.


> 3. also, in getULEBSize() you are doing a signed right shift. Is that
> correct?

This is copy&paste from LLVM's Dwarf code. ULEB cannot have the high
bit set, so the right shift will never pad with ones. (Dwarf 3.0,
ch7.6)


> 4. also I like size_t when you have to return size of something - (in case
> some crazy person wants to emit an attribute more than 2^32 bytes long)
> It wastes memory, but given that we probably will not see lots (as in
> millions) of attributes, its probably ok...

Ok. Done. Committed in 137115.

cheers,
--renato