Skip to content

Work on copy of command to avoid side effect from tokenization #23

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tobiasfrejo
Copy link

Due to tokenization of the parsed command in slash_execute, the string gets mutated on execution.

Example:

cmd = "node 12"
pycsh.slash_execute(cmd)
print(cmd.encode())

prints b'node\x0012' instead of the expected b'node 12'

This is resolved here by copying the command string before it is passed to the slash_execute C function.

Copy link
Contributor

@kivkiv12345 kivkiv12345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!
Before merging however, I'm wondering if we should introduce a verbose argument to control the new "executing" printf.
I don't now much about how this function will be used, but I imagine that the print may become tedious.
On that thought, I should mention that this function is quite divisive. I defend its inclusion, but I can't promise it will stick around.

Another thing, PyCSH has the CLEANUP_FREE macro (from "utils.h"), which I prefer to use for pointers that should be freed when they fall out of scope.
It's not too important on cmd_cpy here, since there aren't any guard clauses where we can forget to free it.

@tobiasfrejo
Copy link
Author

You are right about the printf potentially being tedious. That was just used for debugging, so I have simply removed it now, and also added the CLEANUP_FREE for good measure.

As a defense for keeping the function, I see it as a way to call CSH commands that has not been implemented as Python functions, which we might use in DISCO for automating calls to our own libraries and not having to implement them in Python as well.

@kivkiv12345
Copy link
Contributor

To my dismay, GCC is overly strict about the types of cleanup routines. So using CLEANUP_FREE instead of CLEANUP_STR to free a char* will issue a warning, even though they both call free().
That is to say, you can replace CLEANUP_FREE with CLEANUP_STR, and the warning will go away. Calling free() explicitly here is also fine by me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants