Tensors with proto-indices supported for hashing in a leaf node. #227
Conversation
| @@ -97,6 +97,7 @@ std::wstring deparse_scalar(const Constant::scalar_type& scalar) { | |||
|
|
|||
| std::wstring deparse(const ExprPtr& expr, bool annot_sym) { | |||
| using namespace details; | |||
| if (!expr) return {}; | |||
There was a problem hiding this comment.
I would argue that passing a nullptr into this function is an error
There was a problem hiding this comment.
Creating nullptr-like ExprPtr is very easy as of now (e.g. by default construction). I believe it is convenient to return an empty string for such expressions. I am using this feature to read/write equations to a file for debugging purposes.
There was a problem hiding this comment.
Exactly because it is easy, I think the program should error as soon as possible in order to indicate the part of the code in which you are using a nullptr.
I mean, I'm not against this change - it's just that in my view hiding the use of a nullptr in your code may be a bad idea as it will likely not do what you want it to anyway i.e. will likely cause trouble down the line.
There was a problem hiding this comment.
Empty string -> Nullptr.
Nullptr -> Empty string.
It's a one-to-one mapping. We don't buy much by throwing on empty string parse or nullptr deparse, especially because these functions are convenient functions -- not the core SeQuant features.
There was a problem hiding this comment.
Btw, I called it a bug fix because, previously, the master supported this feature. See here.
No description provided.