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

LGTM. Thanks!

-Jim

On Feb 17, 2012, at 5:04 PM, Derek Schuff <dschuff at google.com> wrote:

> 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>
>> 
>> 
> <mutable.patch>