Kerollmops improvement list

[c0gent]

Trello Project Board

[c0gent]

This feedback is very valuable to me. I don't get enough. I really appreciate this kind of constructive criticism.

• Global:

  • Everything need to work with the Future system
    • I think you mean documentation. Added to TODO.
  • Remove all panics (e.g. Platform)
    • Panics are a useful and important escape hatch for unrecoverable problems: https://doc.rust-lang.org/book/second-edition/ch09-03-to-panic-or-not-to-panic.html. These are programmer errors that cannot be recovered from at runtime.
  • Remove all useless Display implementations
    • [Updated] Display impls are useful for logging, diagnostics, etc.
  • Make every builders become mutable builders
    • In many cases this is impractical, impossible, or unnecessary. Builders come in several valid styles: https://aturon.github.io/ownership/builders.html. Please explain better what you mean or give specific examples.
  • Reduce the visible types at the entrypoint of the doc
    • Added to TODO.
  • Create an Index type/Wrapper
    • Please clarify.
  • Remove useless lifetimes
    • Please provide examples.
  • Add examples usage to a maximum of methods
    • Agreed, more example code is needed throughout the project. Added to TODO.

• Documentation:

  • Add a comment to the Cargo.toml file that reminds to bump the semvers of html_root_url
    • Good idea. Added to TODO.
  • Don't use short/reduced/cutted/unclear names for modules or types (c.g. prm for primitives)
    • Why not? Often a shortened name is perfectly adequate.

• Buffer:

  • Remove the new method
    • Why? The new method is perfectly useful. Do you think it should be private? It may be useful for someone to create a buffer directly rather than use the builder.
  • Why does it have a default_queue ?
    • ::default_queue is used to return a reference to the currently set default queue. It's useful for a variety of purposes, such as ensuring multiple tasks use the same queue without have to store the queue separately.
  • create_sub_buffer don't use SpatialDims for origin
    • Added to TODO.
  • create_sub_buffer reword the documentation
    • The documentation was adapted from the official documentation. It could probably use some work. Added to TODO.
  • Explain why a default_queue can be specified and why it's optional
    • Default queues could be explained better. Added to TODO.
  • Remove from_gl_buffer or change the arguments
    • Why? This is a useful method. What's wrong with the arguments? Please clarify.
  • Change the way read, write methods works, use futures ?
    • Please clarify.

• BufferBuilder:

  • Refacto the context along with the queue method
    • Refactor? Please clarify.
  • Refacto and clarify the host_data method
    • Please clarify.
  • Remove dims and/or use another type to represent the data with 1/2/3 dimensions
    • Added to TODO.
  • Unify fill_val and fill_event because that's the same thing, use futures
    • They are not the same thing and have been separated for good reason. I'll will add more documentation to clarify. Added to TODO.

• BufferCmd:

  • Remove the new method
    • I'll make it private. Added to TODO
  • Remove useless panics when the command operation has already been defined
    • See: 'Remove all panics' above.
  • Remove read_async
    • Added to TODO.
  • Add more informations on the map method
    • Added to TODO.
  • Change the way gl_aquire and gl_release works (e.g. Remove antoher type, more clear)
    • [Updated] Please explain what you think is wrong with these two methods. They appear to function perfectly to me.
  • Why does block is unsafe ?
    • Because setting block to false is potentially unsafe due to asynchronous timings.
  • Refacto the rect method and doesn't panic
    • Please clarify.
  • Remove ewait_opt
    • Added to TODO.
  • Add more explanations to the ewait/enew methods
    • Added to TODO.

• Mem/MapFlags:

  • Follow the std::OpenOptions for ocl::Mem/MapFlags (https://doc.rust-lang.org/std/fs/struct.OpenOptions.html)
    • Please explain. Mem/MapFlags already follow the same pattern (e.g. MemFlags::new().read_only().host_write_only())

• Device:

  • Device::first will panic and don't needs to
    • Added to TODO.

• Platform:

  • Don't use Vec but &[] in list_from_core
    • [Updated] Compiler will reuse the allocation the way it is. Changing the argument to a slice would cause a new allocation. This way the caller decides whether or not to re-allocate (by cloning). Relevant guideline: https://rust-lang-nursery.github.io/api-guidelines/flexibility.html#c-caller-control
  • Don't return raw String from profile and extensions
    • PlatformInfo::Profile and PlatformInfo::Extensions queries returned owned strings.

• Kernel:

  • new use Into<&str> instead of the useless Into<String>
    • Into<String> is far from useless. It allows the caller to pass either an owned string or a reference, avoiding an allocation in the former case.
  • Try to make a better API to arg passing with real typed arguments
    • I agree that the API needs work/redesign. I've always wanted to change it but have never been able to decide on a better design. Any suggestions are very welcome. I'm happy to spend more time discussing this. Added to TODO.
  • Remove unsafe function from top level function
    • Please clarify.
  • Change gwo into set_gwo and get_gwo into gwo
    • Redesign needed. See prior note. Added to TODO.
  • Rename core into as_core
    • All ::core functions need renaming to ::as_core. Added to TODO.
  • Rename by_idx into by_index for more clarity
    • [Updated] The method I assume you're referring to is ::named_arg_idx. To fully expand that method name it would become ::named_argument_index. This is just too verbose. ::named_arg_index doesn't seem like it adds any more clarity at all either. So, on further reflection, the method will remain as it is.

• RwVec:

  • Add an is_empty method to the type
    • [Updated] ::len is already returning stale values and is not to be treated like a usual ::len method call. I'd rather not add more methods to make it seem as though these are somehow equivalent to their slice counterparts because they are not the same. I've added documentation to ::len to explain that the value is immediately stale. Added to TODO (completed).

• EventArray:

  • Remove the push_some
    • Added to TODO.

[c0gent]

Just pinging you, @Kerollmops :)

[Kerollmops]

I'm actually really busy but I added a reminder to add more in this list. I will come back this week probably. 😃

[Kerollmops]

This is very complex to make a clear answer. Their is too many points of conversation and I'm not done with improvements propositions yet. I want to find something like a Trello or one issue by improvement but in another repo to limit the noise for example (i.e. rust-lang/rfcs, crossbeam-rs/rfcs, haskell/rfcs).

[c0gent]

Yeah, I was thinking about making a Github Project for it. Trello would be great too.

[Kerollmops]

And why not RFCs ? Do you think it is overkill or something ?

Because nobody will be able to add issues to the github project or the Trello.

[c0gent]

RFC would be fine, but yes, a bit overkill. In this case I think you should create a Trello board where the two of us can sort out the majority of the small issues. If any large questions remain, we can create an issue in this repo, or create an RFC repo.

[c0gent]

Contact me via: https://gitter.im/cogciprocate/ocl if you need my attention. I usually keep the chat open so it's faster than communicating via github or e-mail :)

[Kerollmops]

We are know talking on this Trello board.