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