Issue 561640043: Abstract Grob property storage into Mutable_properties.
DescriptionAbstract Grob property storage into Mutable_properties.
No functional changes. This change makes it easier to reorganize the storage for mutable properties.
Patch Set 1 #
Total comments: 9
Patch Set 2 : dan #
MessagesTotal messages: 17
https://codereview.appspot.com/561640043/diff/565900043/lily/grob-scheme.cc File lily/grob-scheme.cc (left): https://codereview.appspot.com/561640043/diff/565900043/lily/grob-scheme.cc#o... lily/grob-scheme.cc:326: This should very probably not be part of this patch?
Sign in to reply to this message.
From visual inspection, LGTM.
I only wonder whether we should use
if
{
foo(...);
}
or
if
foo(...);
for single statements – right now, the source code contains both variants...
Sign in to reply to this message.
On 2020/04/13 14:58:01, lemzwerg wrote:
> From visual inspection, LGTM.
>
> I only wonder whether we should use
>
> if
> {
> foo(...);
> }
>
> or
>
> if
> foo(...);
>
> for single statements – right now, the source code contains both variants...
I have been drilled at google to always use the { }, so that's why. Before, we
used the braceless version when it was possible. Standards are wonderful; there
are so many to choose from.
Sign in to reply to this message.
https://codereview.appspot.com/561640043/diff/565900043/lily/grob-scheme.cc File lily/grob-scheme.cc (left): https://codereview.appspot.com/561640043/diff/565900043/lily/grob-scheme.cc#o... lily/grob-scheme.cc:326: On 2020/04/13 14:56:45, hahnjo wrote: > This should very probably not be part of this patch? It's part https://codereview.appspot.com/549840044/ ; removing it here as well was the most expedient way of dealing with a potential merge conflict.
Sign in to reply to this message.
On 2020/04/13 15:06:16, hanwenn wrote: > https://codereview.appspot.com/561640043/diff/565900043/lily/grob-scheme.cc > File lily/grob-scheme.cc (left): > > https://codereview.appspot.com/561640043/diff/565900043/lily/grob-scheme.cc#o... > lily/grob-scheme.cc:326: > On 2020/04/13 14:56:45, hahnjo wrote: > > This should very probably not be part of this patch? > > It's part https://codereview.appspot.com/549840044/ ; removing it here as well > was the most expedient way of dealing with a potential merge conflict. Ah, I missed that the function is referencing one of the changed properties in the return statement. You'll probably still run into merge conflicts, unless you hold off pushing the linked patch until this change has counted down.
Sign in to reply to this message.
On 2020/04/13 16:03:00, hahnjo wrote: > On 2020/04/13 15:06:16, hanwenn wrote: > > https://codereview.appspot.com/561640043/diff/565900043/lily/grob-scheme.cc > > File lily/grob-scheme.cc (left): > > > > > https://codereview.appspot.com/561640043/diff/565900043/lily/grob-scheme.cc#o... > > lily/grob-scheme.cc:326: > > On 2020/04/13 14:56:45, hahnjo wrote: > > > This should very probably not be part of this patch? > > > > It's part https://codereview.appspot.com/549840044/ ; removing it here as well > > was the most expedient way of dealing with a potential merge conflict. > > Ah, I missed that the function is referencing one of the changed properties in > the return statement. You'll probably still run into merge conflicts, unless you > hold off pushing the linked patch until this change has counted down. I since both patches delete the code, I think it will merge cleanly.
Sign in to reply to this message.
https://codereview.appspot.com/561640043/diff/565900043/lily/include/mutable-... File lily/include/mutable-properties.hh (right): https://codereview.appspot.com/561640043/diff/565900043/lily/include/mutable-... lily/include/mutable-properties.hh:31: class Iterator { This is useful, but instead of choosing your own method names, why not give it an interface consistent with standard iterators (where the semantics match)? https://en.cppreference.com/w/cpp/named_req/InputIterator I'd leave out the key() and value() methods and let it dealing with just a generic list of SCM values. If you implemented enough of the iterator interface, and a class to wrap the raw SCM list into an object addressable with begin() and end(), I think you would be able to write range-for statements something like this: for (SCM assoc : ly_list(my_alist)) { SCM key = scm_car(assoc); SCM val = scm_cdr(assoc); ...; } If you're not interested in doing this, I might try it myself. https://codereview.appspot.com/561640043/diff/565900043/lily/include/mutable-... lily/include/mutable-properties.hh:54: Iterator iter() const { Does const serve a purpose here? The iterator doesn't carry through with any kind of enforcement. The same question applies to try_retrieve() and to_alist(). https://codereview.appspot.com/561640043/diff/565900043/lily/include/mutable-... lily/include/mutable-properties.hh:60: void swap(Mutable_properties*); Please: void swap(Mutable_properties&) like everything in the STL.
Sign in to reply to this message.
https://codereview.appspot.com/561640043/diff/565900043/lily/include/mutable-... File lily/include/mutable-properties.hh (right): https://codereview.appspot.com/561640043/diff/565900043/lily/include/mutable-... lily/include/mutable-properties.hh:31: class Iterator { On 2020/04/13 17:15:19, Dan Eble wrote: > This is useful, but instead of choosing your own method names, why not give it > an interface consistent with standard iterators (where the semantics match)? > > https://en.cppreference.com/w/cpp/named_req/InputIterator > > I'd leave out the key() and value() methods and let it dealing with just a > generic list of SCM values. > > If you implemented enough of the iterator interface, and a class to wrap the raw > SCM list into an object addressable with begin() and end(), I think you would be > able to write range-for statements something like this: > > for (SCM assoc : ly_list(my_alist)) > { > SCM key = scm_car(assoc); > SCM val = scm_cdr(assoc); > ...; > } > > If you're not interested in doing this, I might try it myself. By structuring it like this, you enforce the implementation to store the key/value as SCM cells, which is exactly what we want to get away from.
Sign in to reply to this message.
On 2020/04/13 17:22:52, hanwenn wrote: > > If you're not interested in doing this, I might try it myself. > > By structuring it like this, you enforce the implementation to store the > key/value as SCM cells, which is exactly what we want to get away from. OK, I didn't understand that from the description. I still think it would be good for the iterator to have an interface that every C++ programmer expects from an iterator.
Sign in to reply to this message.
On 2020/04/13 17:28:16, Dan Eble wrote: > On 2020/04/13 17:22:52, hanwenn wrote: > > > > If you're not interested in doing this, I might try it myself. > > > > By structuring it like this, you enforce the implementation to store the > > key/value as SCM cells, which is exactly what we want to get away from. > > OK, I didn't understand that from the description. I still think it would be > good for the iterator to have an interface that every C++ programmer expects > from an iterator. What about using the interface that std::unordered_map provides? IIRC the ADT returns a std::pair of (key, value).
Sign in to reply to this message.
https://codereview.appspot.com/561640043/diff/565900043/lily/include/mutable-... File lily/include/mutable-properties.hh (right): https://codereview.appspot.com/561640043/diff/565900043/lily/include/mutable-... lily/include/mutable-properties.hh:31: class Iterator { On 2020/04/13 17:22:52, hanwenn wrote: > On 2020/04/13 17:15:19, Dan Eble wrote: > > This is useful, but instead of choosing your own method names, why not give it > > an interface consistent with standard iterators (where the semantics match)? > > > > https://en.cppreference.com/w/cpp/named_req/InputIterator > > > > I'd leave out the key() and value() methods and let it dealing with just a > > generic list of SCM values. > > > > If you implemented enough of the iterator interface, and a class to wrap the > raw > > SCM list into an object addressable with begin() and end(), I think you would > be > > able to write range-for statements something like this: > > > > for (SCM assoc : ly_list(my_alist)) > > { > > SCM key = scm_car(assoc); > > SCM val = scm_cdr(assoc); > > ...; > > } > > > > If you're not interested in doing this, I might try it myself. > > By structuring it like this, you enforce the implementation to store the > key/value as SCM cells, which is exactly what we want to get away from. I don't see that there is a plan to get away from the value being an SCM cell, just the key (at least explicitly). Am I overlooking something?
Sign in to reply to this message.
On 2020/04/13 20:10:35, dak wrote: > https://codereview.appspot.com/561640043/diff/565900043/lily/include/mutable-... > File lily/include/mutable-properties.hh (right): > > https://codereview.appspot.com/561640043/diff/565900043/lily/include/mutable-... > lily/include/mutable-properties.hh:31: class Iterator { > On 2020/04/13 17:22:52, hanwenn wrote: > > On 2020/04/13 17:15:19, Dan Eble wrote: > > > This is useful, but instead of choosing your own method names, why not give > it > > > an interface consistent with standard iterators (where the semantics match)? > > > > > > https://en.cppreference.com/w/cpp/named_req/InputIterator > > > > > > I'd leave out the key() and value() methods and let it dealing with just a > > > generic list of SCM values. > > > > > > If you implemented enough of the iterator interface, and a class to wrap the > > raw > > > SCM list into an object addressable with begin() and end(), I think you > would > > be > > > able to write range-for statements something like this: > > > > > > for (SCM assoc : ly_list(my_alist)) > > > { > > > SCM key = scm_car(assoc); > > > SCM val = scm_cdr(assoc); > > > ...; > > > } > > > > > > If you're not interested in doing this, I might try it myself. > > > > By structuring it like this, you enforce the implementation to store the > > key/value as SCM cells, which is exactly what we want to get away from. > > I don't see that there is a plan to get away from the value being an SCM cell, > just the key (at least explicitly). Am I overlooking something? The value itself is not necessarily a cell. You can have #t as a value (I am sure you know that.) Making the Key/value combination a cell forces one to store them as such, or to construct a cell on the fly. There is an experiment where I store keys as uint16.
Sign in to reply to this message.
dan
Sign in to reply to this message.
https://codereview.appspot.com/561640043/diff/565900043/lily/include/mutable-... File lily/include/mutable-properties.hh (right): https://codereview.appspot.com/561640043/diff/565900043/lily/include/mutable-... lily/include/mutable-properties.hh:54: Iterator iter() const { On 2020/04/13 17:15:19, Dan Eble wrote: > Does const serve a purpose here? The iterator doesn't carry through with any > kind of enforcement. The same question applies to try_retrieve() and > to_alist(). It allows one to iterate over properties in a const method. What kind of enforcement are you looking for? https://codereview.appspot.com/561640043/diff/565900043/lily/include/mutable-... lily/include/mutable-properties.hh:60: void swap(Mutable_properties*); On 2020/04/13 17:15:19, Dan Eble wrote: > Please: void swap(Mutable_properties&) like everything in the STL. Done.
Sign in to reply to this message.
On 2020/04/13 20:27:34, hanwenn wrote: > On 2020/04/13 20:10:35, dak wrote: > > > https://codereview.appspot.com/561640043/diff/565900043/lily/include/mutable-... > > File lily/include/mutable-properties.hh (right): > > > > > https://codereview.appspot.com/561640043/diff/565900043/lily/include/mutable-... > > lily/include/mutable-properties.hh:31: class Iterator { > > On 2020/04/13 17:22:52, hanwenn wrote: > > > On 2020/04/13 17:15:19, Dan Eble wrote: > > > > This is useful, but instead of choosing your own method names, why not > give > > it > > > > an interface consistent with standard iterators (where the semantics > match)? > > > > > > > > https://en.cppreference.com/w/cpp/named_req/InputIterator > > > > > > > > I'd leave out the key() and value() methods and let it dealing with just a > > > > generic list of SCM values. > > > > > > > > If you implemented enough of the iterator interface, and a class to wrap > the > > > raw > > > > SCM list into an object addressable with begin() and end(), I think you > > would > > > be > > > > able to write range-for statements something like this: > > > > > > > > for (SCM assoc : ly_list(my_alist)) > > > > { > > > > SCM key = scm_car(assoc); > > > > SCM val = scm_cdr(assoc); > > > > ...; > > > > } > > > > > > > > If you're not interested in doing this, I might try it myself. > > > > > > By structuring it like this, you enforce the implementation to store the > > > key/value as SCM cells, which is exactly what we want to get away from. > > > > I don't see that there is a plan to get away from the value being an SCM cell, > > just the key (at least explicitly). Am I overlooking something? > > The value itself is not necessarily a cell. You can have #t as a value (I am > sure you know that.) Sure. Sorry, I assumed you used "cell" colloquially for "SCM" since it wasn't clear to me that you were talking about the association rather than their components. > Making the Key/value combination a cell forces one to store them as such, or to > construct a cell on the fly. Yes, of course. I thought about key or value being SCM, not about their association being accessible as a pair or other form of cell. > There is an experiment where I store keys as uint16. Sure.
Sign in to reply to this message.
On Apr 13, 2020, at 16:31, hanwenn@gmail.com wrote: > >> Does const serve a purpose here? The iterator doesn't carry through > with any >> kind of enforcement. The same question applies to try_retrieve() and >> to_alist(). > > It allows one to iterate over properties in a const method. > > What kind of enforcement are you looking for? There's a tension between C++ and Scheme structures that I struggle with. In C++, a const method usually doesn't change the externally observable state of its object, and *usually* avoids returning a non-const reference to a component part, so as not to open a way for the caller to mutate the object. If this const method is not enforcing these things because SCM data structures have no constness, then what is the value in declaring it const? You've said that you're declaring it const so that it can be called from a const method. Granting that the caller may be const for a good reason, the suggestion that a Mutable_properties object will not be changed by a caller that calls properties.to_alist() is misleading. I think it would be better to declare methods as const where it agrees with reality, e.g. contains(). That way you can meaningfully pass a const Whatever& around and trust that the recipient can not change its state. I would do this even if the consequence is that passing a const Whatever& is not useful at present. In the case that concerns you--iterating the SCM list in a const method of a class that owns a Mutable_properties object—I think I would declare the Mutable_properties object as mutable. That way, all the owner's methods can do as they please with the owned Mutable_properties, consistent with its nature as primarily a SCM structure. In short, that way "const" means const, and "mutable" means mutable. — Dan
Sign in to reply to this message.
On 2020/04/13 22:10:25, dan_faithful.be wrote: > On Apr 13, 2020, at 16:31, mailto:hanwenn@gmail.com wrote: > > > >> Does const serve a purpose here? The iterator doesn't carry through > > with any > >> kind of enforcement. The same question applies to try_retrieve() and > >> to_alist(). > > > > It allows one to iterate over properties in a const method. > > > > What kind of enforcement are you looking for? > > There's a tension between C++ and Scheme structures that I struggle with. In > C++, a const method usually doesn't change the externally observable state of > its object, and *usually* avoids returning a non-const reference to a component > part, so as not to open a way for the caller to mutate the object. > > If this const method is not enforcing these things because SCM data structures > have no constness, then what is the value in declaring it const? You've said > that you're declaring it const so that it can be called from a const method. > Granting that the caller may be const for a good reason, the suggestion that a > Mutable_properties object will not be changed by a caller that calls > properties.to_alist() is misleading. > > I think it would be better to declare methods as const where it agrees with > reality, e.g. contains(). That way you can meaningfully pass a const Whatever& > around and trust that the recipient can not change its state. I would do this > even if the consequence is that passing a const Whatever& is not useful at > present. > > In the case that concerns you--iterating the SCM list in a const method of a > class that owns a Mutable_properties object—I think I would declare the > Mutable_properties object as mutable. That way, all the owner's methods can do > as they please with the owned Mutable_properties, consistent with its nature as > primarily a SCM structure. > > In short, that way "const" means const, and "mutable" means mutable. I view const more as a structured comment than an enforcing mechanism. If we would take a strict stance, I think we would have to delete most of the const keywords, and that would actually increase the chances of developers misusing the APIs.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
