-
Notifications
You must be signed in to change notification settings - Fork 227
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
ExtVar support #77
ExtVar support #77
Conversation
interpreter.go
Outdated
idArrayElement ast.Identifier // TODO what is it? | ||
idInvariant ast.Identifier // TODO what is it? | ||
externalVars vmExtMap // TODO what is it? | ||
// Current stack. It |
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.
Missing word(s) after It
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.
Thanks, fixed
aa3635e
to
e184785
Compare
imports.go
Outdated
func codeToPV(e *evaluator, filename string, code string) potentialValue { | ||
node, err := snippetToAST(filename, code) | ||
if err != nil { | ||
// TODO(sbarzowski) perhaps we should wrap (static) error here |
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.
It should be a runtime error. Current behavior of C++ is broken. We need to document and test
each case (import, importstr, ext-code, tla-code).
It should be allowed to import from a non-existent file in a branch that never gets executed. That is the semantics of the language.
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.
So yeah I think if you want to eagerly pre-parse the AST, you have to defer any syntax error until actual use. The C++ implementation re-parses it every time it's referred to I think?
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.
The C++ implementation re-parses it every time it's referred to I think?
No. I think Go and C++ implementation handle caching in pretty much the same way:
/** Import another Jsonnet file.
*
* If the file has already been imported, then use that version. This maintains
* referential transparency in the case of writes to disk during execution. The
* cache holds a thunk in order to cache the resulting value of execution.
*
* \param loc Location of the import statement.
* \param file Path to the filename.
*/
HeapThunk *import(const LocationRange &loc, const LiteralString *file)
{
ImportCacheValue *input = importString(loc, file);
if (input->thunk == nullptr) {
// EVALUATION CODE
}
return input->thunk;
}
/** Import a file as a string.
*
* If the file has already been imported, then use that version. This maintains
* referential transparency in the case of writes to disk during execution.
*
* \param loc Location of the import statement.
* \param file Path to the filename.
* \param found_here If non-null, used to store the actual path of the file
*/
ImportCacheValue *importString(const LocationRange &loc, const LiteralString *file)
{
std::string dir = dir_name(loc.file);
const UString &path = file->value;
std::pair<std::string, UString> key(dir, path);
ImportCacheValue *cached_value = cachedImports[key];
if (cached_value != nullptr)
return cached_value;
// [code that actually fetches it]
cachedImports[key] = input_ptr;
return input_ptr;
}
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.
Updated TODO to reflect the decision.
e184785
to
750b2d1
Compare
Only through internal API. Also no TLA support at the moment.