http://www.m5sim.org/wiki/index.php/Coding_Style#Naming
On Wed, Sep 16, 2009 at 8:19 AM, nathan binkert <[email protected]> wrote: > Sounds good. I'll update it. > > Nate > > On Wed, Sep 16, 2009 at 8:01 AM, Steve Reinhardt <[email protected]> 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 <[email protected]> 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 <[email protected]> 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<[email protected]> 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<[email protected]> >>>>> 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 >>>>>> [email protected] >>>>>> http://m5sim.org/mailman/listinfo/m5-dev >>>>>> >>>>>> >>>>> _______________________________________________ >>>>> m5-dev mailing list >>>>> [email protected] >>>>> http://m5sim.org/mailman/listinfo/m5-dev >>>>> >>>> _______________________________________________ >>>> m5-dev mailing list >>>> [email protected] >>>> http://m5sim.org/mailman/listinfo/m5-dev >>>> >>>> >>> _______________________________________________ >>> m5-dev mailing list >>> [email protected] >>> http://m5sim.org/mailman/listinfo/m5-dev >>> >> _______________________________________________ >> m5-dev mailing list >> [email protected] >> http://m5sim.org/mailman/listinfo/m5-dev >> >> > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
