On 2020/01/29 11:44:57, dak wrote: > > BTW - I don't want to tell a C++ expert how to use the language in general. If > > we were in an alternate reality where we could start from scratch we could > > reconsider the decision to not use non-const references in structs and > function > > arguments. As it is however, any that we have are most likely errors that we > > should correct. Check > > Errors mean non-intentional things.
This may predate you, but the decision to not store references was intentional, exactly because storing NULL in them is very useful. If you have a reference, it has to be initialized in the constructor, and this introduces a lot of boilerplate because you have to pass the non-null reference across constructors in the class hierarchy. Imagine adding something to the Prob class. If it is a reference, you'll now have to update a dozen classes just so it compiles again. Maybe LilyPond has passed the phase that we need to refactorings, but I remember refactoring constructors being a major PITA. Hence, no references. > My own take here is that we would not want > to use references on things that may be used as SCM values, so things like > > Grob &bla = *unsmob<Grob> (sbla); > > would be quite undesirable. However, for code that has no Schemish > implications, I find it perfectly fine to use C++ references in the manner they > are intended to be used. There has been a recent push to settle on C++11 as a > programming standard to adhere to in order to be more friendly to newcomers, and > it seems sort of pointless to promote C89 standards to go alongside. That makes > people less, not more confident in what they may rely on using. People usually make changes by copy & pasting existing code, and then adapting it to their needs. If there are two ways of structuring the code, this makes things more confusing. > > grep --color -nH -e '&' lily/include/*h|grep -v const > > > > Also, it is rare to check incoming parameters for nullness in implementation. > > I am not exactly sure I'd call that a feature: we had crashes because of it. > Some trampolining code now has the requisite assertions inside since one cannot > perform those checks too late: GCC optimises checks like "if (!this)" away these > days. It is correct that using references makes it harder to do null-pointer derefs, and I am not saying the lack of null checks are a desirable feature. what I am trying to say: For reading the code, it is important for the source code to be consistent with itself. This is also why we have automated formatting. The current code overwhelmingly disavows references. The 2 remaining uses (this being one) stand out like a sore thumb. If anyone wants to modernize the source code to introduce references, I think this should be done with an overall plan of how this will become a consistent idiom. I personally think doing so does not solve pressing problems, but as I won't be doing the work that doesn't bother me so much. My proposal is to go ahead with this change, so I can go then go on with https://github.com/hanwen/lilypond/commit/599878a08c18323810aaa1dee3bf4d9b208e223c which is based on this one. If anyone has a plan for changing pointers to references globally, I am happy to comment on it in a different review thread. Meta question: if I would keep shut for 7 days, is this a change that would go in based on "countdown" ? https://codereview.appspot.com/577410045/