[llvm-commits] PATCH: refactoring MCInstFragment and MCDataFragment

[Jim Grosbach]

On Dec 7, 2012, at 9:50 AM, Eli Bendersky wrote:

> On Fri, Dec 7, 2012 at 9:45 AM, Jim Grosbach <grosbach at apple.com> wrote:
>> This looks pretty reasonable to me.
>> 
>> A small question, though. The default size of the SmallString in MCInstFragment increased from 8 to 32. Why is that? That's where your object size change came from.
>> 
>> -Jim
>> 
> 
> Thanks for taking the time to review this, Jim. The answer to your
> question, I believe, is at least partially in my message:
> 
> "While the increase is insignificant, it can be solved by having the
> fragments hold their data in a SmallVector rather than SmallString.
> This way the interface can use SmallVectorImpl to escape being tied to
> size. Personally I think a SmallVector is more appropriate for such
> uses anyway. This can be implemented in a follow-up patch if
> requested."
> 
> Though I think I should probably elaborate as I may not have been
> clear enough. To have a common interface returning contents,
> getContents() returns SmallString<32>, so the instruction fragment
> should also adhere to that. I'm aware of no way of returning a
> size-independent SmallString, please correct me if I'm wrong. However,
> I think the better way to approach would be to change the SmallString
> to SmallVector, for which we can then use SmallVectorImpl to appease
> the compiler with size-independent interfaces. The SmallString is used
> just for convenience for the code, and I don't think switching to SV
> would be a big loss there. If this sounds reasonable I can sent a
> followup patch fairly soon that fixes this, and brings the size of the
> instruction fragment down to original.

Aha! That's what I get for reading quickly. That makes complete sense, thanks.

Most users of SmallString probably just use StringRef or something like that for access. I agree that SmallVector is more appropriate for this code anyway, so a follow-up sounds great.

-Jim