Yes, I agree the _name designations are meant for clashes between private members and their accessors. I would also prefer that members still use camel case, and I'm not a fan of m_ to designate members. We could think about prepending get/set in front of accessors, but I doubt that will be popular either.
As far as include guards go, your suggestion is fine, although I think any conflicts are currently due to incorrectly formatted include guards. I believe the correct format is supposed to be like: __PATH_IN_GEM5_SRC_TREE_FILE_NAME_HH__ But a lot of people seem to leave off the path name. As far as the macros go, I am not crazy about adding 5 characters either (GEM5_). That said I think we should probably not be using macros for many things we currently use them for; i.e., we should be using inlines, constexpr, and consts. So if we could restrict our usage of macros to the few cases they are needed, then I think requiring GEM5_ for macros would be ok. -----Original Message----- From: gem5-dev [mailto:[email protected]] On Behalf Of Potter, Brandon Sent: Monday, October 10, 2016 3:02 PM To: gem5 Developer List <[email protected]> Subject: Re: [gem5-dev] macro (and identifier) problems in our source code Hi Andreas, I thought that the _name designations were meant for private members with accessors; that's what the style guide says: "Class members that have accessor methods should have a leading underscore to indicate that the user should be using an accessor. The accessor functions themselves should have the same name as the variable without the leading underscore." It's not always an issue with the members being named with an underscore, but I have seen cases with acronyms as the first few characters with full capitalization... so an error according to the C++ standard. The macros in "src/base/misc.hh" that are used extensively are named panic, warn, and fatal. These are examples which are particularly likely to cause conflicts (as I found out this past weekend). The problem manifested as an error after a macro called another macro caused by a conflict on a function name. So, preprocessor sees the "warn" define from "misc.hh" and then sees "warn" from <err.h>. It has already seen the macro so it decides to start replacing the declaration in the <err.h> header file. Since the "warn" macro from "misc.hh" called another macro it was tough to track the problem down; the compiler pointed me to the nested macro and I stared at that code and the surrounding code for a while before realizing the problem with <err.h>. Also, the header that included "misc.hh" was nested in the header file a few includes away. Frankly, it was a waste of time that other developers probably would like to avoid going forward. I'm suggesting added the extra characters so that no one else runs into the problem again (at least as long as we don't include header files from the OTHER gem5 project). I know that it will offset the code and likely cause some line issues that need to be resolved, but it seems like a better programming practice to adopt rather than maintain the status quo and continue to add code. I do agree that it's unfortunate though to need the extra characters However, imagine how nice it would be now if it had been adopted earlier in development. You'd be able to look at a macro and know that it's a macro without needing to resort to header files to verify; also, you'd know that it was unique and need not worry about collisions. Regards, Brandon -----Original Message----- From: gem5-dev [mailto:[email protected]] On Behalf Of Andreas Hansson Sent: Monday, October 10, 2016 4:14 PM To: gem5 Developer List <[email protected]> Subject: Re: [gem5-dev] macro (and identifier) problems in our source code Hi Brandon, No objection on the header include guards. When it comes to private members, I would suggest to just stick with nameOfVar. We have only used _name when there is a constructor parameter name, and it is actually not necessary. It is fine to have the private member be just name, and let the constructor parameter name initialise it with name(name). What macros are causing problems? Adding 5 characters to each and everyone seems rather unfortunate. Andreas On 10/10/2016, 18:52, "gem5-dev on behalf of Potter, Brandon" <[email protected] on behalf of [email protected]> wrote: >Hello all, > >I just posted a patch, http://reviews.gem5.org/r/3650/, which deals >with a name collision between a macro that's defined in our source and >a function declaration included from a standard header. Seems like a >rare collision that I stumbled into, but we should do a better job of >naming our macros given that collisions are a possibility. With macros, >we can't use namespaces to prevent this from happening, but we could >make a rule where we append something like "_GEM5" onto all of the >macros. (It is a much larger problem than function names which is what >the issue is on >RB.) Does anyone have thoughts on this suggestion? If it's a good >suggestion, does anyone want to try their hand at sed magic? I don't >expect this is solvable with a 1-liner, but I don't want to spend time >trying to rename all of our macros. (There are ~300 of them in the >source right now that are function macros; there are a lot of defines.) > >In a related vein, our macro naming should never include __ (double >underscore) anywhere in the macro nor should the macro begin with an >_[A-Z] (underscore followed by uppercase letter). It turns out that our >headers all use __X_Y_HH__ format for our include guards and I've found >a couple of example with variables that use the _[A-Z] format. (It's >not just macros; the rule also applies to any identifier.) We need to >fix these issues. I do not know that it will create a problem given >that we're using the 'HH' in the macro, but it's bad form to continue >on with this. The standard dictates that these forms are reserved for >the implementation of the language so we shouldn't be using them. > >I recommend the following (although I am not volunteering to do it all): > >* Change header guards to have the form: NAME_OF_FILE_HH_ > >* Use m_ (like Ruby) for private members in classes: m_nameOfVar >as opposed to _nameOfVar; (consider something like _PID where the >capitalization seems natural) > >* Append a namespace substitute like "_GEM5" onto the end of >macros (or alternatively use GEM5_ at the beginning) > >Regards, >Brandon >_______________________________________________ >gem5-dev mailing list >[email protected] >http://m5sim.org/mailman/listinfo/gem5-dev IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
