Skip to content

Added ffiCodegen to MakeActions - #3434

Merged
kritzcreek merged 3 commits into
purescript:masterfrom
coot:ffi-codegen
Sep 24, 2018
Merged

Added ffiCodegen to MakeActions#3434
kritzcreek merged 3 commits into
purescript:masterfrom
coot:ffi-codegen

Conversation

@coot

@coot coot commented Sep 20, 2018

Copy link
Copy Markdown
Contributor

There are two reasons for this PR:

  • zephyr can use ffiCodegen to generate ffi code
  • purs ide can show ffi errors (unused ffi imports)

@coot

coot commented Sep 20, 2018

Copy link
Copy Markdown
Contributor Author

@kritzcreek I think you will like this change :)

@kritzcreek

Copy link
Copy Markdown
Member

The change to the IDE code is not necessary, because shushCodegen only affects the second rebuild, which we only use to populate completions and references with non-exported members. The build that actually collects the errors and warnings we send to the editors does generate code. (so we probably actually want to make sure we don't do any extra work on the second build)

That doesn't mean I'm opposed to this change at all ^^ Just that it's not really doing anything for the ide integration.

ma { P.progress = \_ -> pure () }

-- | Stops any kind of codegen (also silences errors about missing or unused FFI
-- files though)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@kritzcreek I was led by this comment. Anyway, the dictionary lookup should not be very expensive (which is the only overhead added to modules without ffi), checkForeignDecls is though, but it's also useful. As I understand, now ffi errors will magically ;) disappear when a module is rebuild.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The comment is misleading, yeah. It's technically not wrong, but we don't care about any errors that might be generated here anyway because we've already gotten all the errors and sent them off to the editor at this point. I'd be totally fine if you just ripped the parenthesized part out.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The first build:

. P.rebuildModule (buildMakeActions
>>= shushProgress $ makeEnv) externs $ m

doesn't shushCodegen.

@kritzcreek kritzcreek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me

@coot

coot commented Sep 23, 2018

Copy link
Copy Markdown
Contributor Author

@kritzcreek I re-started the ci and its green now.

@kritzcreek

Copy link
Copy Markdown
Member

I resolved the conflict introduced by #3433, I'll merge as soon as CI passes.

@kritzcreek
kritzcreek merged commit a8e0911 into purescript:master Sep 24, 2018
@kritzcreek

Copy link
Copy Markdown
Member

Thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants