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

[codegen] conversion from FREE_IMAGE_FORMAT to itself is pointless #72

Closed
timotheecour opened this issue Jan 26, 2019 · 15 comments
Closed
Labels
bug Something isn't working

Comments

@timotheecour
Copy link
Contributor

timotheecour commented Jan 26, 2019

same input example as #71

Hint: conversion from FREE_IMAGE_FORMAT to itself is pointless [ConvFromXtoItselfNotNeeded]

comes from this:

type FREE_IMAGE_FORMAT* = distinct int
converter enumToInt(en: FREE_IMAGE_FORMAT): int {.used.} = en.int
@timotheecour timotheecour changed the title conversion from FREE_IMAGE_FORMAT to itself is pointless [codegen] conversion from FREE_IMAGE_FORMAT to itself is pointless Jan 26, 2019
@zedeus
Copy link

zedeus commented Jan 26, 2019

We talked about this on IRC, make nimterop emit {.hint[ConvFromXtoItselfNotNeeded]: off.}

@timotheecour
Copy link
Contributor Author

why not detect and avoid emitting the converter instead?

@zedeus
Copy link

zedeus commented Jan 26, 2019

That was deemed too complicated for something that could be easily silenced with a pragma, but maybe it's easier now?

@timotheecour
Copy link
Contributor Author

well let's leave this open as a low priority thing

@timotheecour timotheecour changed the title [codegen] conversion from FREE_IMAGE_FORMAT to itself is pointless [low priority] [codegen] conversion from FREE_IMAGE_FORMAT to itself is pointless Jan 26, 2019
@zedeus
Copy link

zedeus commented Jan 26, 2019

The converter has actually been a little problematic for me. ImageMagick uses a MagickBoolean enum for bools (MagickTrue/False), so I made a converter to turn it into bool for my wrapper. The enumToInt converter causes ambiguities in this case though, for example system.not matches both bool and int. What's the purpose of the converter anyway?

@timotheecour
Copy link
Contributor Author

What's the purpose of the converter anyway?

i always wondered! @genotrance ?

@genotrance
Copy link
Collaborator

Nimterop originally generated enums but that doesn't always work for wrapped C enums since they can often be out of order in value. Nim doesn't like that and compile fails.

Instead we simply generate const values. But now, you also need a type since that enum type can be used as params and return values. So you need a converter from the const to int.

Tracking will be tedious since every enum will need to be checked if it is the same type as the one it will be converted from. Easier to just suppress the message.

@zedeus
Copy link

zedeus commented Jan 27, 2019

So, an enum consists of a distinct int type and const variables. What's the point of making what's essentially a distinct int -> int converter? Can you show an example where it's needed?

@genotrance
Copy link
Collaborator

type
  enumtype = distinct int

const
  enumval = 5

proc enumproc(val: enumtype) =
  echo val.int

enumproc(enumval)
a.nim(10, 9) Error: type mismatch: got <int literal(5)>
but expected one of:
proc enumproc(val: enumtype)

expression: enumproc(5)

You get an error cause Nim won't explicitly convert distinct int to int. That's the whole point of distinct. You get to keep all enums of a particular type together and not mixed with other enums.

@genotrance
Copy link
Collaborator

Related conversation.

@timotheecour
Copy link
Contributor Author

timotheecour commented Jan 27, 2019

like @zestyr , I really don’t like the converter being added by default; this should definitely be an opt-in (ie, no default implicit conversion); and this shouldn't be done by converters (it's a too blunt tool)

proposal

  • keep the enum as is, no converter is generated
  • add a nimterop/wrapperutil.nim (or other good name indicating it's meant to be imported by wrappers who want it) with generic procs that make it easy to handle enums the way they want (ie not one size fits all "all enums are convertible to int" type of thing)
    maybe:
import nimterop/wrapperutil
addEnumConverter(MyEnum) # this macro call will add a converter just for `MyEnum`
addEnumBorrow(MyEnum) # this macro call will add `{.borrow.}` for common integer operations; 

(similar to defineEnum from https://gitter.im/nimgen/Lobby?at=5c4dfea3ca428b06450fb8a6 )

but really, i think the best is to use myEnum.int, when needed, from the wrapper writer point of view instead of magic conversions which break type safety distinction in the 1st place

@timotheecour timotheecour changed the title [low priority] [codegen] conversion from FREE_IMAGE_FORMAT to itself is pointless [codegen] conversion from FREE_IMAGE_FORMAT to itself is pointless Jan 27, 2019
@genotrance
Copy link
Collaborator

We cannot delegate everything to the user, it will make the tool hard to use. Already people are struggling with the interface.

We also need this working upfront without which const ENUM2 = ENUM1 << 2 won't work in the wrapper itself, let alone in the users code.

I prefer @zestyr's solution to create a generic template in types. I don't mind types.nim growing because it will be compiled out by Nim if portions are unused.

@genotrance genotrance added the bug Something isn't working label Jan 30, 2019
genotrance added a commit that referenced this issue Jan 30, 2019
@genotrance
Copy link
Collaborator

I've created a branch implementing enums as discussed here. Please review and provide your feedback.

https://github.com/genotrance/nimterop/compare/enumnoncon?expand=1

@zedeus
Copy link

zedeus commented Jan 30, 2019

Looks good, but the pattern can be simplified:

template genOp*(op, typ, output) =
  proc op*(x: typ, y: int): output {.borrow.}
  proc op*(x: int, y: typ): output {.borrow.}
  proc op*(x, y: typ): output {.borrow.}

template defineEnum*(typ: untyped) =
  type
    typ* = distinct int  

  genOp(`+`,   typ, typ)
  genOp(`-`,   typ, typ)
  genOp(`*`,   typ, typ)
  genOp(`<`,   typ, bool)
  genOp(`<=`,  typ, bool)
  genOp(`==`,  typ, bool)
  #genOp(`shl`, typ, typ)
  #genOp(`shr`, typ, typ)
  genOp(`div`, typ, typ)
  genOp(`mod`, typ, typ)

  proc `$`*(x: typ): string {.borrow.}

I've commented out shl/shr because they trigger this error:

enum.nim(19, 8) Error: type mismatch: got <proc [*missing parameters*](x: T: SomeUnsignedInt, y: SomeInteger): T: SomeUnsignedInt{.noSideEffect.}
                                         | proc [*missing parameters*](x: int8, y: SomeInteger): int8{.noSideEffect.}
                                         | proc [*missing parameters*](x: int32, y: SomeInteger): int32{.noSideEffect.}
                                         | proc [*missing parameters*](x: int64, y: SomeInteger): int64{.noSideEffect.}
                                         | proc [*missing parameters*](x: int16, y: SomeInteger): int16{.noSideEffect.}
                                         | proc [*missing parameters*](x: int, y: SomeInteger): int{.noSideEffect.},
                                           type footype, type footype>
but expected one of: 
template genOp(op, typ, output): untyped
  first type mismatch at position: 1
  required type: untyped
  but expression 'shl' is of type: None

expression: genOp(shl, footype, footype)

@genotrance
Copy link
Collaborator

Pushed an update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants