> 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? > > Joel Hestness wrote: > 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.
Thanks Joel. I wish all patches got this level of scrutiny :-). We'll change it to if/else before pushing. - 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
