> 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. > > Joel Hestness wrote: > 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. > > Andreas Hansson wrote: > 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.
Thoughts? - Andreas ----------------------------------------------------------- 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
