[llvm-commits] PATCH: refactoring MCInstFragment and MCDataFragment

Charles Davis cdavis5x at gmail.com
Fri Dec 7 10:18:02 PST 2012


On Dec 7, 2012, at 11:01 AM, Jim Grosbach wrote:

> 
> 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.
Except a SmallString *is* a SmallVector:

/// SmallString - A SmallString is just a SmallVector with methods and accessors
/// that make it work better as a string (e.g. operator+ etc).
template<unsigned InternalLen>
class SmallString : public SmallVector<char, InternalLen> {

You could just have getContents() return a SmallVectorImpl<char>, like you would anyway if you converted the SmallStrings into SmallVectors.

Chip





More information about the llvm-commits mailing list