A swift review of this would be appreciated since mercurial is not playing along very well with this as part of a patch queue.

This is a welcome change. We're still finessing includes-ordering in gem5-gpu, and this change subsumes some of the unofficial policies we're following.

Just a couple minor code style comments below.

util/sort_includes.py (Diff revision 1)

The Python "do X if Y else Z" conditional formatting tends to be unclear and tricky to read, as is the case here. It's also quite uncommon in gem5 (in fact, I'm unable to quickly find another example in the codebase), and using it in line 61 and here mixes code style with the more standard use in analogous line 100. My preference would be to maintain the clearer and consistent use of if-conditionals already common in gem5.

Show all issues

1. Andreas Sandberg Jan. 28, 2015, 3:57 a.m. (Jan. 28, 2015, 3:57 a.m.)

   This is a pretty common Python construct that is completely analogous to the trinary operator in C, which we use a lot. I'm inclined to say that this is a non-issue.

2. Andreas Hansson Jan. 28, 2015, 3:57 a.m. (Jan. 28, 2015, 3:57 a.m.)

   This construct is really vanilla Python these days, and I would suggest to embrace it, assuming no one objects.
   
   On the C++ side we have opted to go for C++11. I think for the Python code we should stay up-to-date as well.

3. Joel Hestness Jan. 28, 2015, 9:45 a.m. (Jan. 28, 2015, 9:45 a.m.)

   While this is analogous in behavior to the C ternary operator, it is very different in readability due to the expression ordering. In the Python case, the condition splits the if and else bodies, which require reading left-to-right, then right-to-left, and then left-to-right again. This is hard to read compared to the C form ((expr) ? result_true : result_false), which reads completely left-to-right. I'm not taking issue with the commonality of the construct in the wild, but rather the readability and commonality in gem5 (Python's structure of this operator is simply less readable than the C structure).
   
   I'd also point out that one of gem5's primary programming philosophies is readability, so "because something is vanilla" probably shouldn't trump "it's more readable". From what I've seen, the C++11 features that we're adopting dramatically improve readability and debuggability, in addition to making the code easier to write (e.g. auto stl types). So, "staying up-to-date" has pretty different meaning for this Python construct versus the C++11 features we're using.
   
   Anyway, this is my piece. Others may disagree with me.

4. Andreas Hansson Jan. 29, 2015, 1:50 a.m. (Jan. 29, 2015, 1:50 a.m.)

   Readability is definitely something we should not sacrifice. I am fully with you on that one, and by no means am I implying that staying up-to-date is more important.
   
   I think in this specific case it comes down to familiarity. I personally do not find the Python ternary operator less readable than if/else (it is closer to how you would express something in a natural language). Given the heated discussions on stackoverflow it's clear that opinions differ, and I understand and respect your point of view.
   
   In the end, if it really is something that you think impairs readability then by all means we'll go ahead and change it to good old if/else.

5. Andreas Hansson Feb. 2, 2015, 9:51 a.m. (Feb. 2, 2015, 9:51 a.m.)

   Thoughts?

6. Joel Hestness Feb. 2, 2015, 11:51 a.m. (Feb. 2, 2015, 11:51 a.m.)

   Looks like we may need to duke it out if no one else has a preference on the subject :P
   
   Yes, I agree that this is a question of familiarity in order to answer whether it should be allowed or avoided in gem5. Based on that, I'd argue that gem5 precedent doesn't support that the Python ternary conditional is familiar to gem5 developers (and probably less likely to be familiar to users). Specifically, I wasn't able to find any Python ternary conditional uses on a more thorough sweep of the existing code. I also looked specifically for if-else statements near return statements and simple assignments, and there are a bunch of cases where the ternary conditional could have been used but isn't (e.g. configs/common/Benchmarks.py:40-49, src/mem/slicc/ast/MachineAST.py:79-82). Some of these examples date as far back as 2006, but most are much more recent, 2009 and later.
   
   This precedent is the case even though the Python ternary conditional is actually a pretty old language feature. From what I can tell, gem5 has been publicly available since late 2003. The Python ternary conditional has been available since Python v2.5 in late 2005 (here's the announcement: https://mail.python.org/pipermail/python-dev/2005-September/056846.html ). This indicates that the ternary conditional is especially not new relative to the age of gem5, so developers have had plenty of time to become familiar and use it if they cared to.
   
   To me, it seems like avoiding the Python ternary conditional is an unofficial gem5 standard, and based on the lack of its use in gem5 historically, I suspect others would be fine that it stay that way.
   
   All of that said, I now have a better familiarity with the construct myself. I could perhaps understand introducing it in limited cases. For instance, when the if-condition is a simple/single statement (e.g. sans compound conditionals), and the if- and else-body expressions are simple (e.g. only single-result assignments/returns, such as in the examples I listed above). It seems likely that anything more complicated could become pretty gnarly to read. In the example from this review request, the compound conditional and the intervening new line ('\') are what trip me up.

7. Andreas Hansson Feb. 2, 2015, 1:09 p.m. (Feb. 2, 2015, 1:09 p.m.)

   Thanks Joel. I wish all patches got this level of scrutiny :-).
   
   We'll change it to if/else before pushing.

util/sort_includes.py (Diff revision 1)

This and lines 154+155 are tricky to read also. The old code was much clearer/easier to step through. Is it important for the code to be this dense?

1. Andreas Sandberg Jan. 28, 2015, 3:57 a.m. (Jan. 28, 2015, 3:57 a.m.)

   I've cleaned up this part of the code (and added a few comments), which should make it more readable. We'll upload new version shortly.

Cool. Looks good to me. Thanks for the style dialog.