Skip to content
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

Generate bindings for defines for variables #62

Merged
merged 6 commits into from
Jun 27, 2018

Conversation

kornilova203
Copy link
Member

During DefineFinderAction save all defines that use an identifier.
During AST traversal find variables declarations and check if there is a define for the variable.
If such define exist then save it in a vector field in IR and generate Scala code for it:

extern int a;
#define A a
  @name("a")
  def A: native.CInt = native.extern

@kornilova203
Copy link
Member Author

Closes #4

@kornilova203 kornilova203 force-pushed the defines-for-extern-variables branch from 5774883 to fb81f8f Compare June 22, 2018 19:13
@kornilova203 kornilova203 force-pushed the defines-for-extern-variables branch from fb81f8f to 312321e Compare June 22, 2018 19:14
@kornilova203 kornilova203 changed the base branch from 0.2 to master June 23, 2018 13:57
@kornilova203 kornilova203 requested a review from jonas June 23, 2018 14:04
@native.extern
object VarDefine {
@name("a")
def A: native.CInt = native.extern
Copy link
Member

@jonas jonas Jun 25, 2018

Choose a reason for hiding this comment

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

I'd expect this be a var since it is not extern const int a?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Yes, it at least should be val
I tried to replace it with var and change value of A but got compiler error
It is not informative:

[error] scala.scalanative.util.UnsupportedException: List(10) (class scala.collection.immutable.$colon$colon)
[error] 	at scala.scalanative.util.package$.unsupported(package.scala:17)
[error] 	at scala.scalanative.nscplugin.NirGenExpr$ExprBuffer.genApplyExternAccessor(NirGenExpr.scala:1572)
[error] 	at scala.scalanative.nscplugin.NirGenExpr$ExprBuffer.genApplyMethod(NirGenExpr.scala:1556)
...

I suppose it is related to this issue scala-native/scala-native#202

So I'll change it to val

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. /me flushing down getopt.h as candidate for #59. 😭

I took a quick look at https://github.com/sjrd/scala-js-ts-importer/blob/master/samples/modifiers.d.ts.scala as inspiration on how to translate variables and it uses val for const and def for readonly members. @sjrd if you have time maybe you can enlighten us on the choices made for the Typescript importer.

In the standard Scala Native bindings, I see def stdin, not sure if this is because it calls a method.

Copy link

Choose a reason for hiding this comment

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

The difference between val and def is that val is stable, i.e., is guaranteed to always return the same value for the same receiver. def can return different values as time passes. This corresponds to const versus readonly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, then it would be better to have val for const values and def for all other

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Thanks for your input @sjrd !

#define C c
#undef C // removed

extern float f;
Copy link
Member

Choose a reason for hiding this comment

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

Why is there no variable generated for f?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because this commit is about defines
Define for f is private __PRIVATE and therefore filtered

@kornilova203 kornilova203 force-pushed the defines-for-extern-variables branch from f3dccf8 to 582d04b Compare June 25, 2018 13:53

extern int c;
#define C c
#undef C // removed
Copy link
Member

Choose a reason for hiding this comment

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

When we will add support for extern int c (#70 this will still generate a def c: native.CInt = extern, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

@native.extern
object VarDefine {
@name("a")
val A: native.CInt = native.extern
Copy link
Member

Choose a reason for hiding this comment

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

When we will support extern variables this could be simply:

def a: native.CInt = extern
def A = a

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but if a variable is private then it will be filtered, so we should be careful here

extern int __private_int;
#define my_int __private_int

@@ -37,4 +37,5 @@ class TreeVisitor : public clang::RecursiveASTVisitor<TreeVisitor> {
virtual bool VisitTypedefDecl(clang::TypedefDecl *tpdef);
virtual bool VisitEnumDecl(clang::EnumDecl *enumdecl);
virtual bool VisitRecordDecl(clang::RecordDecl *record);
virtual bool VisitVarDecl(clang::VarDecl *VD);
Copy link
Member

Choose a reason for hiding this comment

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

🎉

if (!macroName.empty()) {
std::string type = handleReservedWords(
typeTranslator.Translate(varDecl->getType()));
ir.addVarDefine(macroName, varName, type);
Copy link
Member

Choose a reason for hiding this comment

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

This will need to be changed to support extern variables. Ideally everything related with macros/defines would live in possibleVarDefines so we'd end up with:

class Variable : public TypeAndName {
  // ...
}

class VarDefine : public Define {
  // ...
  private:
    std::string defineName; // name of the alias
    std::string variableName; // name of the aliased variable
}

@jonas jonas mentioned this pull request Jun 25, 2018
@kornilova203
Copy link
Member Author

Variable with def gives this error when I am trying to get it's value:

[error] java.lang.NullPointerException
[error] 	at sbt.JUnitXmlTestsListener$TestSuite.$anonfun$stop$2(JUnitXmlTestsListener.scala:92)
[error] 	at scala.collection.TraversableLike.$anonfun$map$1(TraversableLike.scala:234)
[error] 	at scala.collection.immutable.List.foreach(List.scala:389)
...
extern int a;

Binding:

@name("a")
def A: native.CInt = native.extern

Test:

assert(VarDefine.A == 23)

Test succeeds if def is changed to val 😕

@jonas
Copy link
Member

jonas commented Jun 25, 2018

I suspect that def is translated into a method call which blows up because the symbol is not a method. Looks like we have to use val then.

@jonas
Copy link
Member

jonas commented Jun 27, 2018

BTW, it would be good to also update the limitation notice in the README.

@kornilova203 kornilova203 merged commit 8b63ced into master Jun 27, 2018
@kornilova203 kornilova203 deleted the defines-for-extern-variables branch June 27, 2018 04:35
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.

3 participants