[llvm-commits] [llvm] r149918 - in /llvm/trunk: include/llvm/Bitcode/ include/llvm/MC/ include/llvm/Support/ lib/Bitcode/Reader/ lib/Bitcode/Writer/ lib/MC/MCDisassembler/ lib/Support/ lib/Target/ARM/Disassembler/ lib/Target/MBlaze/Disassembler/

[Derek Schuff]

OK. The following patch uses the appropriate mutable members and fixes
the MC const objects, please review:


On Thu, Feb 16, 2012 at 4:29 PM, Jim Grosbach <grosbach at apple.com> wrote:
>
> On Feb 7, 2012, at 3:12 PM, Derek Schuff <dschuff at google.com> wrote:
>
>
>
> On Tue, Feb 7, 2012 at 2:33 PM, Jim Grosbach <grosbach at apple.com> wrote:
>>
>>
>> On Feb 7, 2012, at 2:26 PM, Derek Schuff <dschuff at google.com> wrote:
>>
>> The problem is that fetching more data can cause the underlying structure
>> (e.g. the storage pointer in the example) to be reallocated/resized.
>>
>>
>> That's an implementation detail. It's the exposed interface of the class
>> and the semantics associated that I'm concerned with. As a user of the
>> class, I should neither know nor care what's inside or how it works. In this
>> case, the semantics we want to be able to express are whether the consumer
>> of a MemoryObject can change the underlying data (write to the
>> memory/file/shared object) or not.
>>
>> MutableArrayRef type classes are basically just glorified pointers and
>> designed not to own the data; but the data has to be owned by something
>> accessible to MemoryObject, and MemoryObject::readBytes would have to call a
>> non-const method on whatever does own it. The behavior you're describing is
>> like a const pointer (const char* foo) but the 'const' qualifier on a method
>> basically means 'const char* const foo'
>>
>>
>> It means that the 'this' pointer is const qualified. Any additional
>> semantics of the interface are convention up to the designer.
>
>
> OK. I guess we just had different expectations of what those semantics were.
>
>>
>>
>> We could of course keep const on the methods and do our game-playing by
>> making all the data members of the StreamableMemoryObject mutable, but
>> that's worse (IMO) than just having another class that's like MemoryObject
>> but doesn't derive directly from it.
>>
>>
>> Why?
>>
>
> if all of the data members are mutable, then the 'const' on the methods
> effectively has no meaning at all. or it's a lie.
>
>
> I disagree. Consider, for example, a memory cache on top of a read-only
> memory. The cache itself has to be read-write internally in order to do its
> job, but the interface it exposes the the users of the system is read-only.
> It's a transparent layer between the user of the memory and the memory
> itself. How it does its job, including if it has internal state, is entirely
> an internal detail.
>
> The bitcode streamer is a similar scenario. The consumers don't necessarily
> know or care whether the whole module has been read all at once or
> incrementally. They just want the data. Likewise MemoryObject itself. The
> external interface reflects what underlies the MemoryObject, not how the
> classes derived from MemoryObject actually implement those accesses.
>
> -Jim
>
> More specifically to this case, I think the usage of StreamableMemoryObject
> is different enough from MemoryObject that it shouldn't really be derived,
> and these other issues are even more reason.
> So, I've attached a patch that fixes all the MC stuff and leaves
> MemoryObject as it was, and leaves StreamableMemoryObject as the base class.
> Please review,
>
> thanks,
> -Derek
> <MCFix.patch>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mutable.patch
Type: text/x-patch
Size: 22792 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120217/7b05a932/attachment.bin>