Adding a lrpar::codegen module. - #656
Conversation
This code was previously copied over in the first patch.
This also should have been in the first patch.
Some of these were only needed while we still had codegen outside the module.
|
A couple of oopsies where I copied some code but didn't delete it, or still had things refer to At least I think that is all of it in that the Edit: Anyhow I'm not sure if that helps making it easier to review or not, it's still a lot. |
| let grm = code_gen.grm(); | ||
| let mod_name = build_env.mod_name(); | ||
| let visibility = self.visibility.clone(); | ||
| let visibility = build_env.visibility(); |
There was a problem hiding this comment.
Probably would be better if I squashed all these little changes to output_file into one patch.
| } | ||
| } | ||
|
|
||
| fn extract_ast_validation( |
There was a problem hiding this comment.
I think rather than extract_ast_validation this should be something like derive_ast_validation.
For the other extract functions, I wonder if they would be better named resolve_*?
| fn extract_ast_validation( | ||
| &mut self, | ||
| from_ast: Option<&ASTWithValidityInfo>, | ||
| ) -> Result<ASTWithValidityInfo, Box<dyn Error>> { |
There was a problem hiding this comment.
With my nimbleparse_lsp hat on for that project it wants the raw errors,
so it can send spans directly to the editor for highlighting rather than Box<dyn Error>.
Probably something to think about later
This the beginning of attempt No. 2 at pr #655 just trying to split up the giant commit from that patch into smaller more easily reviewable ones. Below is the original pr description:
Here is an attempt at pulling a codegen module out of
lrpar, it migrates theCTParserBuilderto use it, and passesthe testsuite. I haven't gone over this with a fine toothed comb, there have been some obscure timing related problems I introduced during development that didn't cause any testsuite failures.
(Like calling
check_unused_header_keys()too early before the callback.I wasn't able to do this patch in a way that was even remotely incremental because of ownership issues. Nor really figure out a way to do it in a way that didn't require making slight changes to things as they moved over (mostly changing references to
self, removing callsunwrap()).There are 3 passes to this and a 4th structure
ParserBuildEnvArgs:ParserSrcEnvParserBuildEnvParserCodeGenThe general idea is:
ParserSrcEnv: source, source path, diagnostics generator,headercollection which contains the default values for the%grmtoolssection.ParserBuildEnvArgs, these are essentially the builder arguments including the essentialOption<>types. It's just a minimal builder.ParserBuildEnv: This structure contains fields derived from theBuildEnvArgsand also the inner types from options inBuildEnvArgs. By derived fields I mean things likeASTWithValidationInfoParserCodegen: This one contains aYaccGrammar,StateTable, andStateGraph. In order to generate code, though it still needs theSrcEnvand theBuildEnv.Currently the
BuildEnvtakes ownership of theBuildEnvArgs, it was easiest this way because therebuild_cachecode expectsOptionsit may be we should dropBuildEnvArgsfromBuildEnv.But generally the idea (in a pseudo functional syntax) is something to the effect of:
(SrcEnv?, BuildEnvArgs) -> BuildEnv? -> CodeGen::generate(&build_env, &src_env)?;There is a pretty high probability that this contains some code duplicated from
CTParserBuilder, whichdidn't go fully unused in
CTParserBuilder, given that the diff +/- is only a few hundred lines, I don't expect an enormous amount. But perhaps if it is really used in both places likefn indentappears to be we canuse ctbuilder::indentinstead.I'll try and read through it in this regard tomorrow.