Skip to content

Modular interfaces#4

Merged
lthms merged 12 commits into
lthms:masterfrom
yurug:modular-interfaces
Mar 5, 2019
Merged

Modular interfaces#4
lthms merged 12 commits into
lthms:masterfrom
yurug:modular-interfaces

Conversation

@yurug
Copy link
Copy Markdown
Collaborator

@yurug yurug commented Feb 28, 2019

Here is a draft proposal for define new interfaces to FreeSpec by means of plugins.

The code is neither commented nor stabilized yet but it should give the idea.

@lthms any comment?

@lthms
Copy link
Copy Markdown
Owner

lthms commented Mar 1, 2019

Thanks a lot for this PR, @yurug!

I will do a proper review of the code as soon as possible, although I basically trust you on that side.

Once the code is stabilized, could you add yourself as a contributor of the file you created/modified? Also, an interactive rebase would be neat to squash commits db97a55 in a27e1fc and 5a5a742 in a27e1fc

Copy link
Copy Markdown
Owner

@lthms lthms left a comment

Choose a reason for hiding this comment

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

This is the first time I do a review for FreeSpec :).

Changes look good to me, thanks a lot! Before merging, there remains several small changes to do, I think:

  • Use this occasion to comment a bit the Ocaml files (yours in priority, I can try to comment the exec.ml file latter)
  • Check whether there are missing license headers
  • Add yourself when you contribute to a file, remove my name if you have created the file

I tried to comment your changes throughout my reading of them, hope this will be useful.

Comment thread exec/META Outdated
Comment thread exec/META Outdated
Comment thread exec/src/exec.ml
Comment thread exec/src/interfaces.ml Outdated
* Copyright (C) 2018–2019 ANSSI
*
* Contributors:
* 2019 Thomas Letan <thomas.letan@ssi.gouv.fr>
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I am not the contributor of this file, you are! :)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in cdd7932.

Comment thread exec/src/interfaces.ml Outdated
let register_interfaces interface_initializer =
Queue.add interface_initializer initializers

let force_interface_registering () =
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

force_ sounds a bit “scary.” Can you justify this name when you will comment this PR?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Explained in e8ce114. I hope this will clarify the situation.

Comment thread stdlib/console/META Outdated
Comment thread stdlib/console/META Outdated
package "freespec-stdlib-console" (

description = "Freespec Stdlib/Console Plugin"
version = "8.10"
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Same here as before. Should it be 8.9?

Comment thread stdlib/console/demos/Hello.v
@lthms lthms added the kind | feature Adding something to FreeSpec that cannot be done with existing code label Mar 2, 2019
@lthms
Copy link
Copy Markdown
Owner

lthms commented Mar 5, 2019

Thanks. I think we are good to go. I will just try to run this PR on my laptop to see if everything goes well (I have little doubt), then we can squash everything into a single commit and merge that into master.

@lthms lthms merged commit 1f0340f into lthms:master Mar 5, 2019
lthms pushed a commit that referenced this pull request Mar 5, 2019
In the first iteration of FreeSpec.Exec, the (singleton) set of
interfaces was statically defined by the execution function within the
ocaml code. Supporting an additional interface meant modifying this
function accordingly. With this change, FreeSpec.Exec is now easily
extensible by means of Coq plugins.

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

Labels

kind | feature Adding something to FreeSpec that cannot be done with existing code target | exec target | stdlib

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants