-
Notifications
You must be signed in to change notification settings - Fork 128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Option in bpls
to show the expression string for derived variables
#4145
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only thought here was whether or not we should be returning std::string rather than char* if we're passing outside the Engine class. Inside BP5, or between engine and serializer, we've stuck more closely to C than C++ memory management (for good reason, given the nature of BP5 metadata management and the pain points in BP4 Dims handling). But this would be the first unmanaged pointer returned out of Engine.h. This particular use case isn't a worry at all, and maybe there wouldn't be any other uses, but it's maybe something to think about as we add interfaces.
@eisenhauer I like the idea of returning the value in a Small question: why are we testing the output of |
Completeness? (Really, I have no idea...) |
a18e5a0
to
f24a476
Compare
Can't remember anymore. I think was completeness but probably overkill. |
Would you be able to indent the expression lines under the variable? I.e. 4 spaces for each line |
We could return a |
but then again, if it was already returning a string, I would not complain about wasting memory by this copy |
@pnorbert That's a fair point... This is not bindings, so we're responsible for making sure things are handled safely internally. const or not, the string pointed to by the char* is valid as long as the variable is valid, which on the read size in BP5 is at least from BeginStep to EndStep in streaming mode (longer for RRA) and that's all we need for bpls. So, I'll retract my criticism... |
Fair points, I will return a When the derived variable spans on multiple lines the spaces will only be applied to the first line. @lizdulac is working on having the compressed version of the expression encoded on the writer side so it looks nicer when we plot it. |
f24a476
to
55d3662
Compare
55d3662
to
019557b
Compare
Aaaah, I corrected the text shown in |
No worries! |
Added the option
--show-derived
in bpls to print the expression string for derived variables.