Sounds good. I'll update it. Nate
On Wed, Sep 16, 2009 at 8:01 AM, Steve Reinhardt <ste...@gmail.com> wrote: > I think if we're going to say that some fields have underscores and > some don't, we should stick with saying that it's only the ones that > have accessors that get underscores. Trying to introduce another > dimension like public/private strikes me as something that will just > add to the confusion and reduce compliance. > > One way to look at this is that "normal" code will generally not have > variables with underscores... if it has an underscore, you should be > using the accessor, even if you're in a method where you could access > the field directly. So seeing a reference to a variable that starts > with an underscore is a sign that something special is going on... > either it's an accessor or a constructor (which should be obvious) or > for some reason you're explicitly bypassing the accessor. > > Steve > > On Wed, Sep 16, 2009 at 7:25 AM, nathan binkert <n...@binkert.org> wrote: >> This has been in my inbox forever and I figured I should just do it. >> I guess B is nice since that's the way things are now, the question >> is, what do we want new code to look like? Maybe it should more be >> that private/protected members should start with an underscore and >> publich ones should not. >> >> What do you think? A style change of course does not require that all >> code change overnight. >> >> Nate >> >> On Sun, Aug 2, 2009 at 4:31 PM, Steve Reinhardt <ste...@gmail.com> wrote: >>> Fine with me... one thing to clarify though is whether in the long run: >>> >>> A. all class members should start with underscore just in case we want >>> to give them accessors later >>> >>> or >>> >>> B. only class members with accessors should start with underscore, and >>> others don't >>> >>> I prefer B, mostly because A would be a much more invasive change, and >>> would be harder to enforce, and is generally uglier IMO. However it >>> does get a little confusing in that you end up with classes like >>> Request where privateFlags is about the only member that doesn't start >>> with an underscore. Constructors that allow you to set the initial >>> value of a field that has no accessors get even stranger, since the >>> convention there is that it's the constructor parameter that has the >>> underscore. That may not happen much, but it's still a possibility. >>> >>> A is a little nicer in that you don't have to rename a field when you >>> change your mind about whether you have accessor methods. >>> >>> Anyway, whichever way we decide to go we should make it explicit when >>> you update the style guide. >>> >>> Steve >>> >>> >>> >>> On Sun, Aug 2, 2009 at 1:10 PM, nathan binkert<n...@binkert.org> wrote: >>>> Very awesome. I was tempted to do this a number of times. I'm going >>>> to go ahead and add this standard to the style guide. ok? >>>> >>>> Nate >>>> >>>> On Sat, Aug 1, 2009 at 10:51 PM, Steve Reinhardt<steve.reinha...@amd.com> >>>> wrote: >>>>> changeset 50125d42559c in /z/repo/m5 >>>>> details: http://repo.m5sim.org/m5?cmd=changeset;node=50125d42559c >>>>> description: >>>>> Rename internal Request fields to start with '_'. >>>>> The inconsistency was causing a subtle bug with some of the >>>>> constructors where the params had the same name as the fields. >>>>> This is also a first step to switching the accessors over to >>>>> our new "standard", e.g., getVaddr() -> vaddr(). >>>>> >>>>> diffstat: >>>>> >>>>> 1 file changed, 58 insertions(+), 56 deletions(-) >>>>> src/mem/request.hh | 114 >>>>> ++++++++++++++++++++++++++-------------------------- >>>>> >>>>> diffs (292 lines): >>>>> >>>>> diff -r e46754b929ca -r 50125d42559c src/mem/request.hh >>>>> --- a/src/mem/request.hh Fri Jul 31 10:40:42 2009 -0400 >>>>> +++ b/src/mem/request.hh Sat Aug 01 22:50:10 2009 -0700 >>>>> @@ -132,17 +132,17 @@ >>>>> * The physical address of the request. Valid only if validPaddr >>>>> * is set. >>>>> */ >>>>> - Addr paddr; >>>>> + Addr _paddr; >>>>> >>>>> /** >>>>> * The size of the request. This field must be set when vaddr or >>>>> * paddr is written via setVirt() or setPhys(), so it is always >>>>> * valid as long as one of the address fields is valid. >>>>> */ >>>>> - int size; >>>>> + int _size; >>>>> >>>>> /** Flag structure for the request. */ >>>>> - Flags flags; >>>>> + Flags _flags; >>>>> >>>>> /** Private flags for field validity checking. */ >>>>> PrivateFlags privateFlags; >>>>> @@ -155,15 +155,15 @@ >>>>> Tick _time; >>>>> >>>>> /** The address space ID. */ >>>>> - int asid; >>>>> + int _asid; >>>>> >>>>> /** The virtual address of the request. */ >>>>> - Addr vaddr; >>>>> + Addr _vaddr; >>>>> >>>>> /** >>>>> * Extra data for the request, such as the return value of >>>>> * store conditional or the compare value for a CAS. */ >>>>> - uint64_t extraData; >>>>> + uint64_t _extraData; >>>>> >>>>> /** The context ID (for statistics, typically). */ >>>>> int _contextId; >>>>> @@ -171,10 +171,13 @@ >>>>> int _threadId; >>>>> >>>>> /** program counter of initiating access; for tracing/debugging */ >>>>> - Addr pc; >>>>> + Addr _pc; >>>>> >>>>> public: >>>>> - /** Minimal constructor. No fields are initialized. */ >>>>> + /** Minimal constructor. No fields are initialized. >>>>> + * (Note that _flags and privateFlags are cleared by Flags >>>>> + * default constructor.) >>>>> + */ >>>>> Request() >>>>> {} >>>>> >>>>> @@ -218,23 +221,23 @@ >>>>> * allocated Request object. >>>>> */ >>>>> void >>>>> - setPhys(Addr _paddr, int _size, Flags _flags, Tick time) >>>>> + setPhys(Addr paddr, int size, Flags flags, Tick time) >>>>> { >>>>> - assert(_size >= 0); >>>>> - paddr = _paddr; >>>>> - size = _size; >>>>> + assert(size >= 0); >>>>> + _paddr = paddr; >>>>> + _size = size; >>>>> _time = time; >>>>> >>>>> - flags.clear(~STICKY_FLAGS); >>>>> - flags.set(_flags); >>>>> + _flags.clear(~STICKY_FLAGS); >>>>> + _flags.set(flags); >>>>> privateFlags.clear(~STICKY_PRIVATE_FLAGS); >>>>> privateFlags.set(VALID_PADDR|VALID_SIZE); >>>>> } >>>>> >>>>> void >>>>> - setPhys(Addr _paddr, int _size, Flags _flags) >>>>> + setPhys(Addr paddr, int size, Flags flags) >>>>> { >>>>> - setPhys(_paddr, _size, _flags, curTick); >>>>> + setPhys(paddr, size, flags, curTick); >>>>> } >>>>> >>>>> /** >>>>> @@ -242,18 +245,17 @@ >>>>> * allocated Request object. >>>>> */ >>>>> void >>>>> - setVirt(int _asid, Addr _vaddr, int _size, Flags _flags, Addr _pc) >>>>> + setVirt(int asid, Addr vaddr, int size, Flags flags, Addr pc) >>>>> { >>>>> - assert(_size >= 0); >>>>> - asid = _asid; >>>>> - vaddr = _vaddr; >>>>> - size = _size; >>>>> - flags = _flags; >>>>> - pc = _pc; >>>>> + assert(size >= 0); >>>>> + _asid = asid; >>>>> + _vaddr = vaddr; >>>>> + _size = size; >>>>> + _pc = pc; >>>>> _time = curTick; >>>>> >>>>> - flags.clear(~STICKY_FLAGS); >>>>> - flags.set(_flags); >>>>> + _flags.clear(~STICKY_FLAGS); >>>>> + _flags.set(flags); >>>>> privateFlags.clear(~STICKY_PRIVATE_FLAGS); >>>>> privateFlags.set(VALID_VADDR|VALID_SIZE|VALID_PC); >>>>> } >>>>> @@ -265,10 +267,10 @@ >>>>> * to guarantee that the size and flags are also set. >>>>> */ >>>>> void >>>>> - setPaddr(Addr _paddr) >>>>> + setPaddr(Addr paddr) >>>>> { >>>>> assert(privateFlags.isSet(VALID_VADDR)); >>>>> - paddr = _paddr; >>>>> + _paddr = paddr; >>>>> privateFlags.set(VALID_PADDR); >>>>> } >>>>> >>>>> @@ -280,14 +282,14 @@ >>>>> { >>>>> assert(privateFlags.isSet(VALID_VADDR)); >>>>> assert(privateFlags.noneSet(VALID_PADDR)); >>>>> - assert(split_addr > vaddr && split_addr < vaddr + size); >>>>> + assert(split_addr > _vaddr && split_addr < _vaddr + _size); >>>>> req1 = new Request; >>>>> *req1 = *this; >>>>> req2 = new Request; >>>>> *req2 = *this; >>>>> - req1->size = split_addr - vaddr; >>>>> - req2->vaddr = split_addr; >>>>> - req2->size = size - req1->size; >>>>> + req1->_size = split_addr - _vaddr; >>>>> + req2->_vaddr = split_addr; >>>>> + req2->_size = _size - req1->_size; >>>>> } >>>>> >>>>> /** >>>>> @@ -303,7 +305,7 @@ >>>>> getPaddr() >>>>> { >>>>> assert(privateFlags.isSet(VALID_PADDR)); >>>>> - return paddr; >>>>> + return _paddr; >>>>> } >>>>> >>>>> /** >>>>> @@ -319,7 +321,7 @@ >>>>> getSize() >>>>> { >>>>> assert(privateFlags.isSet(VALID_SIZE)); >>>>> - return size; >>>>> + return _size; >>>>> } >>>>> >>>>> /** Accessor for time. */ >>>>> @@ -342,14 +344,14 @@ >>>>> getFlags() >>>>> { >>>>> assert(privateFlags.isSet(VALID_PADDR|VALID_VADDR)); >>>>> - return flags; >>>>> + return _flags; >>>>> } >>>>> >>>>> void >>>>> - setFlags(Flags _flags) >>>>> + setFlags(Flags flags) >>>>> { >>>>> assert(privateFlags.isSet(VALID_PADDR|VALID_VADDR)); >>>>> - flags.set(_flags); >>>>> + _flags.set(flags); >>>>> } >>>>> >>>>> /** Accessor function for vaddr.*/ >>>>> @@ -357,7 +359,7 @@ >>>>> getVaddr() >>>>> { >>>>> assert(privateFlags.isSet(VALID_VADDR)); >>>>> - return vaddr; >>>>> + return _vaddr; >>>>> } >>>>> >>>>> /** Accessor function for asid.*/ >>>>> @@ -365,7 +367,7 @@ >>>>> getAsid() >>>>> { >>>>> assert(privateFlags.isSet(VALID_VADDR)); >>>>> - return asid; >>>>> + return _asid; >>>>> } >>>>> >>>>> /** Accessor function for asi.*/ >>>>> @@ -373,7 +375,7 @@ >>>>> getAsi() >>>>> { >>>>> assert(privateFlags.isSet(VALID_VADDR)); >>>>> - return flags & ASI_BITS; >>>>> + return _flags & ASI_BITS; >>>>> } >>>>> >>>>> /** Accessor function for MMAPED_IPR flag. */ >>>>> @@ -381,14 +383,14 @@ >>>>> isMmapedIpr() >>>>> { >>>>> assert(privateFlags.isSet(VALID_PADDR)); >>>>> - return flags.isSet(MMAPED_IPR); >>>>> + return _flags.isSet(MMAPED_IPR); >>>>> } >>>>> >>>>> void >>>>> setMmapedIpr(bool r) >>>>> { >>>>> assert(VALID_VADDR); >>>>> - flags.set(MMAPED_IPR); >>>>> + _flags.set(MMAPED_IPR); >>>>> } >>>>> >>>>> /** Accessor function to check if sc result is valid. */ >>>>> @@ -403,14 +405,14 @@ >>>>> getExtraData() const >>>>> { >>>>> assert(privateFlags.isSet(VALID_EXTRA_DATA)); >>>>> - return extraData; >>>>> + return _extraData; >>>>> } >>>>> >>>>> /** Accessor function for store conditional return value.*/ >>>>> void >>>>> - setExtraData(uint64_t _extraData) >>>>> + setExtraData(uint64_t extraData) >>>>> { >>>>> - extraData = _extraData; >>>>> + _extraData = extraData; >>>>> privateFlags.set(VALID_EXTRA_DATA); >>>>> } >>>>> >>>>> @@ -447,31 +449,31 @@ >>>>> getPC() const >>>>> { >>>>> assert(privateFlags.isSet(VALID_PC)); >>>>> - return pc; >>>>> + return _pc; >>>>> } >>>>> >>>>> /** Accessor Function to Check Cacheability. */ >>>>> - bool isUncacheable() const { return flags.isSet(UNCACHEABLE); } >>>>> - bool isInstFetch() const { return flags.isSet(INST_FETCH); } >>>>> - bool isPrefetch() const { return flags.isSet(PREFETCH); } >>>>> - bool isLLSC() const { return flags.isSet(LLSC); } >>>>> - bool isLocked() const { return flags.isSet(LOCKED); } >>>>> - bool isSwap() const { return flags.isSet(MEM_SWAP|MEM_SWAP_COND); } >>>>> - bool isCondSwap() const { return flags.isSet(MEM_SWAP_COND); } >>>>> + bool isUncacheable() const { return _flags.isSet(UNCACHEABLE); } >>>>> + bool isInstFetch() const { return _flags.isSet(INST_FETCH); } >>>>> + bool isPrefetch() const { return _flags.isSet(PREFETCH); } >>>>> + bool isLLSC() const { return _flags.isSet(LLSC); } >>>>> + bool isLocked() const { return _flags.isSet(LOCKED); } >>>>> + bool isSwap() const { return _flags.isSet(MEM_SWAP|MEM_SWAP_COND); } >>>>> + bool isCondSwap() const { return _flags.isSet(MEM_SWAP_COND); } >>>>> >>>>> bool >>>>> isMisaligned() const >>>>> { >>>>> - if (flags.isSet(NO_ALIGN_FAULT)) >>>>> + if (_flags.isSet(NO_ALIGN_FAULT)) >>>>> return false; >>>>> >>>>> - if ((vaddr & 0x1)) >>>>> + if ((_vaddr & 0x1)) >>>>> return true; >>>>> >>>>> - if (flags.isSet(NO_HALF_WORD_ALIGN_FAULT)) >>>>> + if (_flags.isSet(NO_HALF_WORD_ALIGN_FAULT)) >>>>> return false; >>>>> >>>>> - if ((vaddr & 0x2)) >>>>> + if ((_vaddr & 0x2)) >>>>> return true; >>>>> >>>>> return false; >>>>> _______________________________________________ >>>>> m5-dev mailing list >>>>> m5-dev@m5sim.org >>>>> http://m5sim.org/mailman/listinfo/m5-dev >>>>> >>>>> >>>> _______________________________________________ >>>> m5-dev mailing list >>>> m5-dev@m5sim.org >>>> http://m5sim.org/mailman/listinfo/m5-dev >>>> >>> _______________________________________________ >>> m5-dev mailing list >>> m5-dev@m5sim.org >>> http://m5sim.org/mailman/listinfo/m5-dev >>> >>> >> _______________________________________________ >> m5-dev mailing list >> m5-dev@m5sim.org >> http://m5sim.org/mailman/listinfo/m5-dev >> > _______________________________________________ > m5-dev mailing list > m5-dev@m5sim.org > http://m5sim.org/mailman/listinfo/m5-dev > > _______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
