-
Notifications
You must be signed in to change notification settings - Fork 47
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
support on-demand parse for typedef-shadowed files #237
Conversation
@@ -2134,18 +2134,19 @@ describe('BrsFile', () => { | |||
|
|||
describe('type definitions', () => { | |||
it('only exposes defined functions even if source has more', async () => { | |||
await program.addOrReplaceFile('source/main.d.bs', ` |
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.
Swapped the order of the file loading, so we can ensure the file was fully parsed (since it didn't know about the .d.bs file)
public get parser() { | ||
if (!this._parser) { | ||
//remove the typedef file (if it exists) | ||
this.hasTypedef = false; |
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.
these need to be removed before calling parse
, because parse
will stop parsing if it detects a typedef file. It's a bit of a hack, but since it's all internal logic, it works fine.
|
||
//if there is no typedef file, and this file hasn't been parsed yet, parse it now | ||
//(solves issue when typedef gets deleted and this file had skipped parsing) | ||
if (!this.hasTypedef && this.wasParseSkipped) { |
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.
This is no longer necessary, since the parser is a getter, it will parse the file on-demand the first time it's needed.
/** | ||
* Was parsing skipped because the file has a typedef? | ||
*/ | ||
private wasParseSkipped = false; |
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.
not necessary now that parser is a getter.
@@ -891,8 +893,9 @@ export class BrsFile { | |||
|
|||
//consume tokens backwards until we find someting other than a dot or an identifier | |||
let tokens = []; | |||
for (let i = this.parser.tokens.indexOf(currentToken); i >= 0; i--) { | |||
currentToken = this.parser.tokens[i]; | |||
const parser = this.parser; |
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.
lift the parser out of the loop so we only pay the getter cost once.
@@ -932,8 +935,9 @@ export class BrsFile { | |||
} | |||
|
|||
public getPreviousToken(token: Token) { | |||
let idx = this.parser.tokens.indexOf(token); | |||
return this.parser.tokens[idx - 1]; | |||
const parser = this.parser; |
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.
lift the parser out of the loop so we only pay the getter cost once.
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.
Could use _parser
no?
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.
No, that's not safe. This is a public method, which provides no guarantees about who will be calling it. Could be a plug-in, in which case _parser might be undefined.
import util from './util'; | ||
|
||
export let globalFile = new BrsFile('global', 'global', null); | ||
globalFile.parser = new Parser(); | ||
globalFile.parser.parse([]); | ||
globalFile.parse(''); |
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.
simplify the global file a bit
Plugins and external consumers of
.brs
/.bs
files should not need to know whether a file parsing has been skipped due to typedef loading. This PR addresses the issue by converting theBrsFile.parser
into a getter, and parsing the file on-demand if the parsing was previously skipped.