misc feedback

[timotheecour]

• SizeType* = uint is incorrect
  use SizeType* = csize_t instead (only incorrect in case std::vector is created with a custom size_type, but your API doesn't expose that anyways, and it'd be a niche case)

• swap is missing; it's pretty useful for performance (edit by @kaushalmodi: moved to Add wrapper for swap #2)

• things like first can SIGSEGV in case container is empty; probably needs when compileOption("boundChecks"): ... as done in some procs in https://github.com/BigEpsilon/nim-cppstl/blob/master/src/cppstl/vector.nim ; but maybe let's hold off until Collaborate on the std::vector wrapper? Clonkk/nim-cppstl#3 is resolved

[timotheecour]

• simplify

when defined(c) or defined(js) or defined(objc):
  {.error: "This library needs to be compiled with the cpp backend.".}

=> static: doAssert defined(cpp)
=> doesn't even need an error msg, it'll be self evident if fails

[timotheecour]

• nit: single quotes are now supported in nim's doc comments, and simpler is better (esp since ppl are used to markdown)

This has an alias proc ``size``.
=>
This has an alias proc `size`.

[timotheecour]

• replace code-block:: with runnableExamples

=> you'll get syntax highlighting for free when reading source code, syntax is more concise (no need for newline etc) and code block still has some limitations (thanks for addressing some of them in nim-lang/Nim#14306, there's still --doccmd:-d:foo that's not supported)

(this one is important IMO)

[timotheecour]

• urls should be part of the doc comments eg:

# https://en.cppreference.com/w/cpp/container/vector/front
proc first*[T](v: Vector[T]): var T {.importcpp: "front".}

=>

proc first*[T](v: Vector[T]): var T {.importcpp: "front".}
  ## https://en.cppreference.com/w/cpp/container/vector/front
  ## ...

and being lazy is ok here with just the raw uril, unless you want to use the rst (ugly...) syntax like:

`front <https://en.cppreference.com/w/cpp/container/vector/front>`_ 

[kaushalmodi]

static: doAssert defined(cpp)

I had tried that, but it fails on running nim doc ... I get this.

/home/kmodi/usr_local/apps/6/nim/devel/lib/system/fatal.nim(49, 5) Error: unhandled exception: /home/kmodi/sandbox/nim/std_vector/src/std_vector.nim(1, 18) defined(cpp) [AssertionDefect]

Also, I like the error here as it gives a simple one-liner error, and not a backtrace. So it looks (as it should) more like a user-error and not a crash.

My preferred way would have been:

when not defined(cpp):
  {.error: "This library needs to be compiled with the cpp backend.".}

But it fails with a similar error when running nim doc .. (even with that --backednd:cpp switch):

/home/kmodi/sandbox/nim/std_vector/src/std_vector.nim(2, 10) Error: This library needs to be compiled with the cpp backend.

[kaushalmodi]

replace code-block:: with runnableExamples

runnableExamples does not work when the proc has no body (like in the case of just importcpp-only wrappers.

Example:

proc len*(v: Vector): SizeType {.importcpp: "#.size()".}
  ## Return the number of elements in the Vector.
  ##
  ## This has an alias proc `size`.
  ##
  ## .. code-block::
  ##    :test:
  ##    var
  ##      v = newVector[int]()
  ##    doAssert v.size() == 0
  ##
  ##    v.add(100)
  ##    v.add(200)
  ##    doAssert v.len() == 2
  ##
  ## https://en.cppreference.com/w/cpp/container/vector/size

For the cases where the proc is not just importcpp based wrapper, I have used runnableExamples.

[timotheecour]

runnableExamples does not work when the proc has no body

just add =, it works :-) but maybe we shd clarify this in manual.

I have yet to find a single use case for code-block:: inside nim sources (even for code not really meant for being run, one can always use when false or if false etc)

[timotheecour]

I had tried that, but it fails on running nim doc ... I get this.

ok i see the problem; this works:

import std/compilesettings
static: doAssert querySetting(backend) == "cpp" # or error, that's orthogonal and a taste issue; i'm usually in favor of lazy+DRY but fine

but IMO defined(cpp) should work too... investigating...

[kaushalmodi]

but IMO defined(cpp) should work too... investigating...

Thanks for that.

[kaushalmodi]

swap is missing; it's pretty useful for performance

Moved this to a separate issue.

[kaushalmodi]

@timotheecour After adding the trailing = to the proc declarations, I get this warning for each declaration:

Warning: Cannot prove that 'result' is initialized. This will become a compile time error in the future. [ProveInit]

Do you know a way to avoid that warning?

[timotheecour]

• it's a nim bug, and sounds not hard to fix; we should file it;
• {.push warning[ProveInit]:off.} in std_vector didn't help
• switch("warning", "ProveInit:off") in config.nims won't help much since that config.nims isn't read if i'm doing import pkg/std_vector from another project

[timotheecour]

ok i fixed nim-lang/Nim#14314 and it's in devel :)

[kaushalmodi]

simplify when defined(c) or defined(js) or defined(objc): ..

This is now when not defined(cpp):

https://github.com/Clonkk/nim-cppstl/blob/29777c7137efa9f13de01b058286d4e16c8ae104/cppstl/std_vector.nim#L8

[kaushalmodi]

but maybe let's hold off until Collaborate on the std::vector wrapper? Clonkk/nim-cppstl#3 is resolved

I will now be archiving this repo as I have resolved that issue. This repo content is now merged into nim-cppstl.

[kaushalmodi]

Closing this issue as all check-box items are complete.