On Thu, Jan 28, 2016 at 1:53 AM Andreas Sandberg <[email protected]> wrote:
> Hi Steve, > > On 26/01/2016, 22:44, "gem5-dev on behalf of Steve Reinhardt" < > [email protected]<mailto:[email protected]> on behalf of > [email protected]<mailto:[email protected]>> wrote: > > - There's currently an 'hg m5format' command in addition to the 'hg > m5style' command. Is there any point in maintaining both of these? > Strangely enough they are independent functions. Prior to my patch, only > m5format checked spaces after if/for/while. After my patch, I think the > only thing that m5format checks that m5style doesn't is line length. > Unless there's a reason to keep it, I'd like to get rid of m5format. > > Merging them sounds like a reasonable approach to me. The only practical > reason for keeping them separate is that the style checker (m5style) > generally corrects style issues, while m5format only lists formatting > violations. I'm not sure if that makes any difference for most use cases > though. > Yea, I'm not sure there's a useful distinction, and even if there is, it's not clear the current code follows it... for example, whitespace checking is done in both m5style and m5format, and the space after if/for/while check is only done in m5format despite it being more of a style thing IMO. Plus if you've got finer-grain control over individual checks then there's even less need to segregate them. Not to mention that it's just annoying that they're basically independent pieces of code that happen to be in the same file. > - There's also a bunch of code at the bottom of style.py to allow you to > run it directly as a python script (and not just as a mercurial plug-in). > However, as far I can tell, most if not all of that code is broken. Is > there really a need to maintain this? I can see where it would be nice, so > I'd definitely keep it if it worked, but at this point I'd rather just > delete it than have to fix it. > > I would very much like to keep this functionality and possibly even extend > it a bit. We are planning a switch to git internally, so this would be > required if we are going to be able to run the style checker. > Good point, though I think "fix" or even "rewrite" might be more appropriate verbs than "extend". So if you switch to git, will you be able to install this script as a hook as we do with mercurial? As I mentioned, the thing that prompted me to dig into this is the apparently increasing number of style errors that are creeping into the committed code, and it would be good to try to close down the paths that allow that to happen. Steve _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
