> 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.
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. - 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
