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
