Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
commit 5a4039b700f3a7447401780c720070d14e2891bd Author: Han-Wen Nienhuys Date: Fri Jan 31 08:24:44 2020 +0100 Clean up embedded scheme parsing/evaluation. Renames and reorders functions to clarify the mechanism. No consequential functional changes. Separates input and output parameters. https://codereview.appspot.com/577410045/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
https://sourceforge.net/p/testlilyissues/issues/5737/ https://codereview.appspot.com/577410045/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
On 2020/01/31 18:33:09, hanwenn wrote: > On 2020/01/31 18:22:47, Dan Eble wrote: > > On 2020/01/31 17:52:45, hanwenn wrote: > > > you can do a local alias > > > > > > vector<> = *vec; > > > > > > to aid readability. > > > > The more I think about banning non-const reference parameters, the more I'm > > against it. Google's coding standards may work for them, but their rationale* > > for this one is weak. How can we resolve this disagreement quickly? Do you > > simply have the final say as the project founder? > > Can we have this discussion on a thread separate from this code review? > I want this code to go in. This code is a definite improvement in my book. I like the names of the functions, and it seems to me that eliminating the Pars_start class is a good idea. Han-Wen has responded well to comments (even making changes that are not his preferred way of doing things). This patch LGTM. I would like to see some separate discussion about the status of Input and the use of non-constant reference pointers. But we shouldn't hold up this patch to have that discussion. Carl https://codereview.appspot.com/577410045/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
On Fri, Jan 31, 2020 at 7:43 PM Dan Eble wrote: > > Can we have this discussion on a thread separate from this code review? > > I want this code to go in. > > Well, you can't prevent people from replying to their email, but neither can > their replies prevent you from pushing commits. Well, I'm trying to follow the established process here which involves waiting for the code review to be LGTM'd, after which other magical steps happen. Or do I get to bypass process? -- Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
On Jan 31, 2020, at 13:33, hanw...@gmail.com wrote: > > On 2020/01/31 18:22:47, Dan Eble wrote: >> On 2020/01/31 17:52:45, hanwenn wrote: >>> you can do a local alias >>> >>> vector<> = *vec; >>> >>> to aid readability. >> >> The more I think about banning non-const reference parameters, the > more I'm >> against it. Google's coding standards may work for them, but their > rationale* >> for this one is weak. How can we resolve this disagreement quickly? > Do you >> simply have the final say as the project founder? > > Can we have this discussion on a thread separate from this code review? > I want this code to go in. Well, you can't prevent people from replying to their email, but neither can their replies prevent you from pushing commits. — Dan
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
On 2020/01/31 18:22:47, Dan Eble wrote: > On 2020/01/31 17:52:45, hanwenn wrote: > > you can do a local alias > > > > vector<> = *vec; > > > > to aid readability. > > The more I think about banning non-const reference parameters, the more I'm > against it. Google's coding standards may work for them, but their rationale* > for this one is weak. How can we resolve this disagreement quickly? Do you > simply have the final say as the project founder? Can we have this discussion on a thread separate from this code review? I want this code to go in. https://codereview.appspot.com/577410045/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
On 2020/01/31 17:52:45, hanwenn wrote: > you can do a local alias > > vector<> = *vec; > > to aid readability. The more I think about banning non-const reference parameters, the more I'm against it. Google's coding standards may work for them, but their rationale* for this one is weak. How can we resolve this disagreement quickly? Do you simply have the final say as the project founder? * "References can be confusing, as they have value syntax but pointer semantics." https://codereview.appspot.com/577410045/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
On 2020/01/31 17:38:55, Dan Eble wrote:
> On 2020/01/30 23:22:46, hanwenn wrote:
> > In the lily/ directory
> >
> > git grep 'vector<[^>]\+> &' *c|grep -v const
> >
> > returns 20 results, which is pretty small, given the number of
methods in the
> > code base. A cursory inspection suggests that Mike introduced most
of these,
> and
> > I would have probably suggested to use pointers there too. I would
also change
> > these signatures if would stumble across them while refactoring
something
> else.
>
> Wouldn't this policy tend toward crimes against readability like the
following?
>
> void twiddle_vector(vector *vec)
> {
> if (!vec || vec->empty ())
> return;
>
> for (size_t i = 0; i < vec->size() - 1; ++i)
> {
> if ((*vec)[i] < 10)
> (*vec)[i] = (*vec)[i] + 2 * (*vec)[i + 1];
> else
> (*vec)[i] = (*vec)[i] * 2 - (*vec)[i + 1];
> }
> }
>
> How would you approach that?
you can do a local alias
vector<> = *vec;
to aid readability.
https://codereview.appspot.com/577410045/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
On 2020/01/30 23:22:46, hanwenn wrote:
> In the lily/ directory
>
> git grep 'vector<[^>]\+> &' *c|grep -v const
>
> returns 20 results, which is pretty small, given the number of methods
in the
> code base. A cursory inspection suggests that Mike introduced most of
these, and
> I would have probably suggested to use pointers there too. I would
also change
> these signatures if would stumble across them while refactoring
something else.
Wouldn't this policy tend toward crimes against readability like the
following?
void twiddle_vector(vector *vec)
{
if (!vec || vec->empty ())
return;
for (size_t i = 0; i < vec->size() - 1; ++i)
{
if ((*vec)[i] < 10)
(*vec)[i] = (*vec)[i] + 2 * (*vec)[i + 1];
else
(*vec)[i] = (*vec)[i] * 2 - (*vec)[i + 1];
}
}
How would you approach that?
https://codereview.appspot.com/577410045/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
ezt írta (időpont: 2020. jan. 31., P, 11:55): > > > https://codereview.appspot.com/577410045/diff/581560047/lily/parse-scm.cc > File lily/parse-scm.cc (right): > > https://codereview.appspot.com/577410045/diff/581560047/lily/parse-scm.cc#newcode77 > lily/parse-scm.cc:77: const Input *hi = >start_; > On 2020/01/31 10:49:13, benko.pal wrote: > > I understand (and like) adding the const, but can't understand > changing the > > reference to pointer even after reading the whole discussion. > > the change from & to * here is merely syntactical. of course it is; but to me it's nearer to obfuscation than to cleanup.
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
https://codereview.appspot.com/577410045/diff/581560047/lily/parse-scm.cc File lily/parse-scm.cc (right): https://codereview.appspot.com/577410045/diff/581560047/lily/parse-scm.cc#newcode77 lily/parse-scm.cc:77: const Input *hi = >start_; On 2020/01/31 10:49:13, benko.pal wrote: > I understand (and like) adding the const, but can't understand changing the > reference to pointer even after reading the whole discussion. the change from & to * here is merely syntactical. For me, the discussion around & vs * is about using non-const & in function signatures and as class members. However, what I want most at this point is to submit this change, so I can get on with the next change in the series. https://codereview.appspot.com/577410045/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
https://codereview.appspot.com/577410045/diff/581560047/lily/parse-scm.cc File lily/parse-scm.cc (right): https://codereview.appspot.com/577410045/diff/581560047/lily/parse-scm.cc#newcode77 lily/parse-scm.cc:77: const Input *hi = >start_; I understand (and like) adding the const, but can't understand changing the reference to pointer even after reading the whole discussion. https://codereview.appspot.com/577410045/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
Adapted description.
On Fri, Jan 31, 2020 at 8:36 AM Han-Wen Nienhuys wrote:
>
> On Fri, Jan 31, 2020 at 8:30 AM Han-Wen Nienhuys wrote:
> > Locally, I have
> >
> > Clean up embedded scheme parsing/evaluation.
> >
> > Renames and reorders functions to clarify the mechanism. No
> > consequential functional changes.
> >
> > Separates input and output parameters.
> >
> > but I can't find a button to edit the change description.
>
> found it. It is in the corner ("edit issue"). It would be nice if
> git-cl upload did this automatically.
>
> --
> Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen
--
Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
On Fri, Jan 31, 2020 at 8:30 AM Han-Wen Nienhuys wrote:
> Locally, I have
>
> Clean up embedded scheme parsing/evaluation.
>
> Renames and reorders functions to clarify the mechanism. No
> consequential functional changes.
>
> Separates input and output parameters.
>
> but I can't find a button to edit the change description.
found it. It is in the corner ("edit issue"). It would be nice if
git-cl upload did this automatically.
--
Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
On Fri, Jan 31, 2020 at 1:31 AM wrote: > > On 2020/01/30 23:22:46, hanwenn wrote: > > I feel this whole discussion has gone out of hand, and in the interest > of > > expediency, I have replaced > > > > const Input* > > > > with > > > > Input > > > > in the class declaration, so somebody can give this an LGTM now. > > Please read the following with a friendly tone of voice. Thanks for keeping a cool head ! > I'm having the same trouble I initially had trying to match the > description of this change with its content. "Renames and reorders > functions to clarify the mechanism. No functional > changes." Yet in patch set 4, there is a difference other than renaming > and reordering. Parse_start makes a copy of the Input that it didn't > before, and it also has an additional Input member. I don't have the > background knowledge to say whether this discrepancy is consequential, It is not. > just that it doesn't match the description; but that keeps me from > saying "looks good." Maybe someone else can. Either way, I don't think > it's too much to ask to expand the description to reflect the change > more accurately. Locally, I have Clean up embedded scheme parsing/evaluation. Renames and reorders functions to clarify the mechanism. No consequential functional changes. Separates input and output parameters. but I can't find a button to edit the change description. -- Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
On 2020/01/30 23:22:46, hanwenn wrote: > I feel this whole discussion has gone out of hand, and in the interest of > expediency, I have replaced > > const Input* > > with > > Input > > in the class declaration, so somebody can give this an LGTM now. Please read the following with a friendly tone of voice. I'm having the same trouble I initially had trying to match the description of this change with its content. "Renames and reorders functions to clarify the mechanism. No functional changes." Yet in patch set 4, there is a difference other than renaming and reordering. Parse_start makes a copy of the Input that it didn't before, and it also has an additional Input member. I don't have the background knowledge to say whether this discrepancy is consequential, just that it doesn't match the description; but that keeps me from saying "looks good." Maybe someone else can. Either way, I don't think it's too much to ask to expand the description to reflect the change more accurately. https://codereview.appspot.com/577410045/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
On 2020/01/30 14:31:07, dan_faithful.be wrote: > On Jan 30, 2020, at 04:54, mailto:hanw...@gmail.com wrote: > > > > 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. > > The discussion has turned from (a) passing a required parameter as a reference, > to (b) using a reference as a member variable. (a) does not imply (b). You can > pass in a T& and store it in a T* to avoid the constraints that (b) would place > on the use of your class (though they apparently were not previously a problem > in this case). > > > The current code overwhelmingly disavows references. The 2 remaining > > uses (this being one) stand out like a sore thumb. > > We must be miscommunicating, because I see a lot more than 2. For some > examples, There are (were) 2 instance of non-const references passed in .hh files. One dead method in Score_performer, and Lily_lexer::scan_word In the lily/ directory git grep 'vector<[^>]\+> &' *c|grep -v const returns 20 results, which is pretty small, given the number of methods in the code base. A cursory inspection suggests that Mike introduced most of these, and I would have probably suggested to use pointers there too. I would also change these signatures if would stumble across them while refactoring something else. Note that const& are OK as an optimization of call by value; in the caller, the effect of cont& and call by value is indistinguishable. I feel this whole discussion has gone out of hand, and in the interest of expediency, I have replaced const Input* with Input in the class declaration, so somebody can give this an LGTM now. I also have the feeling we spend a lot of time and energy talking past each other. How about a short voice/video call between the 3 of us to make sure we are on the same page? cheers, > > git grep 'vector<[^>]\+> &' > > Please clarify. > > Thanks, > — > Dan > https://codereview.appspot.com/577410045/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
On Jan 30, 2020, at 04:54, hanw...@gmail.com wrote: > > 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. The discussion has turned from (a) passing a required parameter as a reference, to (b) using a reference as a member variable. (a) does not imply (b). You can pass in a T& and store it in a T* to avoid the constraints that (b) would place on the use of your class (though they apparently were not previously a problem in this case). > The current code overwhelmingly disavows references. The 2 remaining > uses (this being one) stand out like a sore thumb. We must be miscommunicating, because I see a lot more than 2. For some examples, git grep 'vector<[^>]\+> &' Please clarify. Thanks, — Dan
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
On 2020/01/30 09:54:48, hanwenn wrote: > 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. That assumes that every variable is created equal, and every purpose is the same. > 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. That assumes that references are being used within a class hierarchy. > Imagine adding something to the Prob class. We are not talking about the Prob class. We are not looking for one-size-fits-all solutions, but for a solution matching the problem. > If it is a reference, you'll now have to update a dozen classes just > so it compiles again. There is no reason whatsoever that the existing constructors in the Prob class should not cater for initializing the reference. I agree that references in data structures can be a problem, and very much so if the data structure is supposed to have a persistent life time, like basically all Smobs do, because references have a life time tied to the referenced objects. But that's not what we are talking about here: here we have a parameter packet with a life-time exactly bounded by the call. Basically you state that we should not be using tools appropriate to the job we need to solve, but restrict ourselves to a common denominator. With a language as large as C++, which has grown to its size exactly because there are a lot of things not solved well by existing tools, that is not going to be a good fit. We recently decided to bump our C++ standard up to C++11 in order to enable a more idiomatic programming style for newcomers, and yet you argue for restricting use of the most basic C++ programming techniques regardless of the context they are used in. Prescribing a rigid mixture of archaic and modern is not likely to increase the attractiveness for LilyPond contributors in general. > Maybe LilyPond has passed the phase that we need to refactorings, > but I remember refactoring constructors being a major PITA. Hence, > no references. This sounds like "my way or the highway". That is a valid and in some manners efficient way of structuring a project top-down, but at the current point of time LilyPond's leadership model is more based on consensus-finding. If changing that is desired in order to get rid of cumbersome discussions, I think it should at least be put up to debate. > > 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 = *unsmob (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. C++ is not Lua. It just doesn't have one-size-fits-all constructs, so there will always either be a considerable number of tools in play, or the code becomes non-idiomatic and cumbersome to work with. > what I am trying to say: > > For reading the code, it is important for the source code to be consistent with > itself. C++ is what it is, and it is what we have to work with. Consistency for me does not mean that I refrain from using references where appropriate but that I refrain from littering code all over the place, like with copy For example, consistency for me means that all accesses to Scheme data structures now without exception run through smobs.hh and associated header and C files. That is why disabling Scheme marking passes in all of LilyPond can be done by outcommenting a single line in all of the source code. It is also why changing the overall GC schemes is feasible. > This is also why we have automated formatting. Nope. We have automated formatting so that people reading the code do not get irritated by _gratuitous_ differences, asking themselves "why is this different?". That is different from having differences with
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
https://codereview.appspot.com/577410045/diff/551410043/lily/parse-scm.cc File lily/parse-scm.cc (right): https://codereview.appspot.com/577410045/diff/551410043/lily/parse-scm.cc#newcode59 lily/parse-scm.cc:59: On 2020/01/29 14:42:22, dak wrote: > Pouring oil on the fire... Rietveld highlighting indicates non-empty lines > consisting only of spaces. Incidentally, those would already get highlighted > with > > git log --check Done. added a hook to my .emacs to delete them. https://codereview.appspot.com/577410045/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
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 = *unsmob (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/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
https://codereview.appspot.com/577410045/diff/551410043/lily/parse-scm.cc File lily/parse-scm.cc (right): https://codereview.appspot.com/577410045/diff/551410043/lily/parse-scm.cc#newcode59 lily/parse-scm.cc:59: Pouring oil on the fire... Rietveld highlighting indicates non-empty lines consisting only of spaces. Incidentally, those would already get highlighted with git log --check https://codereview.appspot.com/577410045/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
On 2020/01/29 06:36:07, hanwenn 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. 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 = *unsmob (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. > 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. https://codereview.appspot.com/577410045/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
On 2020/01/28 23:38:24, Dan Eble wrote: > On 2020/01/28 22:06:33, hanwenn wrote: > > > > In general, pointers are preferred in function signatures, so you can see that > > the value is mutated in the call site. See also > > https://google.github.io/styleguide/cppguide.html#Reference_Arguments > > OK, they're preferred by one or more people at Google. They're not preferred by > me, the people I've worked with, or the creator of C++: > https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-ptr-ref I've never been a big fan of Stroustrup, so we are both coherent, at least :-) (see question 9 https://developers.slashdot.org/story/00/02/25/1034222/c-answers-from-bjarne-stroustrup) > > What would we do in a check? Passing a null input is a programming error; if > we > > don't check, we'll generate a segfault and that seems appropriate for a > > programming error. > > Not seeing a satisfying way to handle a null pointer is a hint that using a > reference would be better. You've preferred one of two kinds of run-time > errors, but the choice you've dismissed is a compile-time error when someone > tries to pass a pointer to a function that requires a reference. A compile-time > error is an improvement over a run-time error, isn't it? > > I don't want to spend a great deal time trying to change your mind, and I'm > certainly not going to tell a project founder what to do here, but I would rest > easier if there were at least a comment on the prototype that the pointer is not > allowed to be null. I reorganized a bit more; I hope you like it better now. 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 grep --color -nH -e '&' lily/include/*h|grep -v const Also, it is rare to check incoming parameters for nullness in implementation. https://codereview.appspot.com/577410045/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
On 2020/01/28 22:06:33, hanwenn wrote: > > In general, pointers are preferred in function signatures, so you can see that > the value is mutated in the call site. See also > https://google.github.io/styleguide/cppguide.html#Reference_Arguments OK, they're preferred by one or more people at Google. They're not preferred by me, the people I've worked with, or the creator of C++: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-ptr-ref > What would we do in a check? Passing a null input is a programming error; if we > don't check, we'll generate a segfault and that seems appropriate for a > programming error. Not seeing a satisfying way to handle a null pointer is a hint that using a reference would be better. You've preferred one of two kinds of run-time errors, but the choice you've dismissed is a compile-time error when someone tries to pass a pointer to a function that requires a reference. A compile-time error is an improvement over a run-time error, isn't it? I don't want to spend a great deal time trying to change your mind, and I'm certainly not going to tell a project founder what to do here, but I would rest easier if there were at least a comment on the prototype that the pointer is not allowed to be null. https://codereview.appspot.com/577410045/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
https://codereview.appspot.com/577410045/diff/571430047/lily/include/parse-scm.hh File lily/include/parse-scm.hh (right): https://codereview.appspot.com/577410045/diff/571430047/lily/include/parse-scm.hh#newcode30 lily/include/parse-scm.hh:30: SCM parse_embedded_scheme (Input *i, bool safe, Lily_parser *parser); On 2020/01/28 22:06:32, hanwenn wrote: > On 2020/01/27 13:39:31, Dan Eble wrote: > > Changing Input& to Input* is more than cosmetic. Input& requires an object, > but > > Input* admits a nullptr. I'm concerned that I don't see that any checks have > > been added before the pointer is dereferenced. > > This code stores a reference, which is completely out of style in LilyPond. It's pretty untypical, not least because unsmobbing works on pointers. That being said, Input is likely the least coherently treated data structure in that regard if I remember correctly, quite often getting passed by copy (as seen in the signature of evaluate_embedded_scheme). Part of the reason for this use may be that for Scheme protection to set in, you need to actually smobify the structure and then keep an SCM value around in the stack somewhere which may prove inconvenient. Having a local copy and passing a reference around is quite more static regarding the life times. Things go as far as Input() creating a "null input" without location data, so the value "no input" is not actually represented by using a null pointer. All that peculiarity of Input was not invented by me but originally there. Part of the reason may be that parser.yy needs a fixed data type for its @$ and similar location data, so the C structure interface inherent there might have been partly responsible for this somewhat peculiar usage. I don't have a particular opinion on this usage myself but think that the current use of a reference might have been done by me trying to keep with the general spirit of how Input is getting used without losing sight of C++. Not necessarily successfully so, but I doubt there were a lot of reviewers advising me at the time. https://codereview.appspot.com/577410045/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
https://codereview.appspot.com/577410045/diff/571430047/lily/include/parse-scm.hh File lily/include/parse-scm.hh (right): https://codereview.appspot.com/577410045/diff/571430047/lily/include/parse-scm.hh#newcode30 lily/include/parse-scm.hh:30: SCM parse_embedded_scheme (Input *i, bool safe, Lily_parser *parser); On 2020/01/27 13:39:31, Dan Eble wrote: > Changing Input& to Input* is more than cosmetic. Input& requires an object, but > Input* admits a nullptr. I'm concerned that I don't see that any checks have > been added before the pointer is dereferenced. This code stores a reference, which is completely out of style in LilyPond. In general, pointers are preferred in function signatures, so you can see that the value is mutated in the call site. See also https://google.github.io/styleguide/cppguide.html#Reference_Arguments What would we do in a check? Passing a null input is a programming error; if we don't check, we'll generate a segfault and that seems appropriate for a programming error. https://codereview.appspot.com/577410045/
Re: Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
https://codereview.appspot.com/577410045/diff/571430047/lily/include/parse-scm.hh File lily/include/parse-scm.hh (right): https://codereview.appspot.com/577410045/diff/571430047/lily/include/parse-scm.hh#newcode30 lily/include/parse-scm.hh:30: SCM parse_embedded_scheme (Input *i, bool safe, Lily_parser *parser); Changing Input& to Input* is more than cosmetic. Input& requires an object, but Input* admits a nullptr. I'm concerned that I don't see that any checks have been added before the pointer is dereferenced. https://codereview.appspot.com/577410045/
Clean up embedded scheme parsing/evaluation. (issue 577410045 by hanw...@gmail.com)
Reviewers: , Message: this is preparation for https://github.com/hanwen/lilypond/commit/599878a08c18323810aaa1dee3bf4d9b208e223c Description: Clean up embedded scheme parsing/evaluation. Renames and reorders functions to clarify the mechanism. No functional changes. Please review this at https://codereview.appspot.com/577410045/ Affected files (+88, -87 lines): M lily/include/parse-scm.hh M lily/lexer.ll M lily/parse-scm.cc M lily/undead.cc
