Skip to content
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

Replace externs::eval with externs::none #7646

Merged
merged 1 commit into from May 2, 2019

Conversation

Projects
None yet
4 participants
@illicitonion
Copy link
Contributor

commented May 1, 2019

This is the only use of eval.

@illicitonion illicitonion requested review from stuhood and blorente May 1, 2019

@stuhood

stuhood approved these changes May 1, 2019

@stuhood

This comment has been minimized.

Copy link
Member

commented May 1, 2019

While I can definitely imagine other usages of eval, I'm fine with adding them back later.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

While I can definitely imagine other usages of eval, I'm fine with adding them back later.

Not here, but in general, we can consider #[allow(dead_code)] methods with a clear comment if we want to allow things to exist as example code, e.g. for demonstrating some complex FFI operation. I think in this case we likely don't want to have example code for using eval because of the fact that eval is evil, so not applicable here.

@stuhood

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Eval is not particularly evil. It gets a bad rap when it's used to execute user-specified code, and thus becomes a security concern. But if the rust code wanted to eval an entire block of code that it specified, it could do that.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

I actually totally agree, but was indirectly trying to confirm I didn't think that adding the #[allow(dead_code)] bit was a necessary change to make to this PR.

@blorente
Copy link
Contributor

left a comment

Thanks!

@@ -452,12 +452,6 @@ def extern_call(self, context_handle, func, args_ptr, args_len):
args = tuple(c.from_value(arg[0]) for arg in self._ffi.unpack(args_ptr, args_len))
return self.call(c, runnable, args)

@_extern_decl('PyResult', ['ExternContext*', 'uint8_t*', 'uint64_t'])

This comment has been minimized.

Copy link
@blorente

blorente May 2, 2019

Contributor

Maybe leave a NB: We can use eval and this is how it was implemented <link to this PR> here or somewhere else, to make it easy to re-add later? Not sure it's worth, but just a thought.

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 2, 2019

Author Contributor

People can look through the git history if they think we may have implemented it before and want to re-add it :)

Replace externs::eval with externs::none
This is the only use of eval.

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/externs/none branch from 3acffc9 to 89381ca May 2, 2019

@illicitonion illicitonion merged commit 761d4c9 into pantsbuild:master May 2, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@illicitonion illicitonion deleted the twitter:dwagnerhall/externs/none branch May 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.