I updated this some more... there were some minor mistakes I wanted to fix, then I got carried away with the example.
Question: our guide says that consts are supposed to look like other vars, and I added one to the example consistent with that, but in fact we don't stick to it very well: % find . -name '*.hh' | xargs grep 'static const' Many/most of these are ALL_CAPS cases where we've replaced a #define with a const and kept the old name, but there are a number of others that use MixedCase (like types) and others that use Mixed_Case_With_Underscores. I personally like the last alternative, since it's unique yet not as obnoxious as all caps. Are we committed to the idea of having constants be indistinguishable from variables? Steve On Wed, Sep 16, 2009 at 8:26 AM, nathan binkert <[email protected]> wrote: > 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 > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
