Skip to content

Reuse generated bindings #140

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 8, 2018
Merged

Reuse generated bindings #140

merged 1 commit into from
Aug 8, 2018

Conversation

@kornilova203
Copy link
Member

@kornilova203 kornilova203 commented Jul 30, 2018 •
edited
Loading

Closes #2

Compilation requires https://github.com/nlohmann/json library.

Format of config file changed to json. It is very similar to previous stdHeaders file, but it provides a way to specify custom names mapping:

{
  "_stdio.h": "scala.scalanative.native.stdio",
  "hiredis.h": {
    "object": "bindings.hiredis.hiredis",
    "names": {
      "struct redisContext": "RedisContext"
    }
  }
}

To do:

  • validate config file
  • support custom name mapping (e.g struct s -> s instead of struct_s)
  • test function pointer type (clang replaces typedef with actual function pointer type, so it's not possible to reuse function pointer type)
  • update documentation
  • add the option to sbt plugin

@kornilova203 kornilova203 changed the base branch from recursive-structs to 0.3 July 31, 2018 06:54
@kornilova203 kornilova203 mentioned this pull request Jul 31, 2018
Closed
@kornilova203 kornilova203 force-pushed the reuse-bindings branch 3 times, most recently from 11fcf04 to 4735226 Compare August 2, 2018 14:26
@kornilova203 kornilova203 changed the base branch from 0.3 to check-exit-code August 2, 2018 14:26
@kornilova203 kornilova203 force-pushed the reuse-bindings branch 2 times, most recently from 200ab75 to 85fedc2 Compare August 4, 2018 06:58
@kornilova203 kornilova203 changed the base branch from check-exit-code to master August 4, 2018 06:58
@kornilova203 kornilova203 force-pushed the reuse-bindings branch 13 times, most recently from 81de677 to bfa9529 Compare August 6, 2018 05:52
@kornilova203 kornilova203 requested a review from jonas August 6, 2018 06:47
@kornilova203 kornilova203 changed the title [WIP] Reuse generated bindings Reuse generated bindings Aug 6, 2018
jonas
jonas reviewed Aug 6, 2018
View reviewed changes
Copy link
Member

@jonas jonas left a comment

Choose a reason for hiding this comment

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

Looks good although I hope we can figure out a way to make this change work with the PR to use bindings options in the sbt plugin.

bindgen/Main.cpp Outdated
"link", llvm::cl::cat(Category),
llvm::cl::desc("Library to link with, e.g. -luv for libuv"));
llvm::cl::opt<std::string> ReuseBindingsConfig(
"integrate-bindings", llvm::cl::cat(Category),
Copy link
Member

Choose a reason for hiding this comment

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

--binding-config might be better

bindgen/Main.cpp

auto reuseBindingsConfig = ReuseBindingsConfig.getValue();
if (!reuseBindingsConfig.empty()) {
locationManager.loadConfig(reuseBindingsConfig);
Copy link
Member

Choose a reason for hiding this comment

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

Any thoughts on also providing a built-in default config to ease use of the standard library bindings from Scala Native

Copy link
Member Author

@kornilova203 kornilova203 Aug 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It may be a separate option, for example, useStandardScalaNativeBindings: Boolean

I looked through native package and found types only in stdio and in complex (but bindgen fails to translate float complex, double complex etc types, so currently it's not possible to reuse complex).

The easiest solution would be to hard code standard config in C++ code and "activate" it by passing use-standard-scala-native-bindings option.

Or it is possible to support multiple config files, so sbt plugin can pass to bindgen a user-defined config(s) and a standard config

Copy link
Member Author

Choose a reason for hiding this comment

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

I created an issue #147

bindgen/ir/IR.cpp Outdated

template <typename T>
bool IR::hasOutputtedDeclaration(
bool IR::hasOutputtedType(
Copy link
Member

Choose a reason for hiding this comment

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

shouldOutoutType for consistency?

bindgen/ir/LocationManager.cpp
s << line;
}
config = json::parse(s.str());
validateConfig(config);
Copy link
Member

Choose a reason for hiding this comment

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

Great that you put validation of the configuration. ❤️

bindgen/ir/LocationManager.cpp Outdated
std::string object = headerEntry.get<std::string>();
if (object.empty()) {
throw std::invalid_argument("Invalid configuration. Each header "
"entry should contain non empty "
Copy link
Member

Choose a reason for hiding this comment

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

I believe it is "non-empty'

bindgen/ir/TypeDef.cpp Outdated
}
if (!type) {
if (startsWith(name, "struct_")) {
return "struct " +
Copy link
Member

Choose a reason for hiding this comment

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

So we still encode the name with an underscore internally?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that it's better to store types with spaces

tools/src/main/scala/org/scalanative/bindgen/Bindgen.scala Outdated
}

def integrateBindings(config: File): Bindgen = {
require(config.exists())
Copy link
Member

Choose a reason for hiding this comment

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

Please add a proper error message here.

tools/src/main/scala/org/scalanative/bindgen/Bindgen.scala Outdated
withArgs("--extra-arg-before", extraArgBefore) ++
integrateBindingsConfig
.flatMap(c => Option(Seq("--integrate-bindings", c.getAbsolutePath)))
.getOrElse(Seq[String]()) ++
Copy link
Member

Choose a reason for hiding this comment

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

Didn't withArgs work here?

withArgs("--...", integrateBindingsConfig)

Copy link
Member Author

Choose a reason for hiding this comment

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

withArgs("--binding-config", bindingConfig.map(_.getAbsolutePath)) does work

@kornilova203 kornilova203 mentioned this pull request Aug 6, 2018
Open
@kornilova203 kornilova203 force-pushed the reuse-bindings branch 2 times, most recently from fe73222 to 04a5d59 Compare August 7, 2018 12:08
@kornilova203
Reuse generated bindings
4b91635 
Use json format for config file
Update documentation
@kornilova203 kornilova203 merged commit ab51928 into master Aug 8, 2018
@kornilova203 kornilova203 deleted the reuse-bindings branch August 8, 2018 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jonas jonas jonas left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kornilova203 @jonas