[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/

[Jim Grosbach]

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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120216/24e088cc/attachment.html>