> On Jan. 27, 2015, 11:57 p.m., Joel Hestness wrote: > > util/sort_includes.py, line 75 > > <http://reviews.gem5.org/r/2614/diff/1/?file=43359#file43359line75> > > > > 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. > > Andreas Sandberg wrote: > 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. > > Andreas Hansson wrote: > 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.
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. - Joel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2614/#review5804 ----------------------------------------------------------- On Jan. 28, 2015, 11:56 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2614/ > ----------------------------------------------------------- > > (Updated Jan. 28, 2015, 11:56 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10669:e82b485546d1 > --------------------------- > style: Update the style checker to handle new include order > > As of August 2014, the gem5 style guide mandates that a source file's > primary header is included first in that source file. This helps to > ensure that the header file does not depend on include file ordering > and avoids surprises down the road when someone tries to reuse code. > > In the new order, include files are grouped into the following blocks: > * Primary header file (e.g., foo.hh for foo.cc) > * Python headers > * C system/stdlib includes > * C++ stdlib includes > * Include files in the gem5 source tree > > Just like before, include files within a block are required to be > sorted in alphabetical order. > > This changeset updates the style checker to enforce the new order. > > > Diffs > ----- > > util/sort_includes.py 3c42be107634 > util/style.py 3c42be107634 > > Diff: http://reviews.gem5.org/r/2614/diff/ > > > Testing > ------- > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
