c-blake / cligen

Nim library to infer/generate command-line-interfaces / option / argument parsing; Docs at
https://c-blake.github.io/cligen/
ISC License
501 stars 23 forks source link

Should this work with cpp backend? #178

Closed pb-cdunn closed 3 years ago

pb-cdunn commented 3 years ago
% nim --version
Nim Compiler Version 1.5.1 [Linux: amd64]
Compiled at 2020-12-03
Copyright (c) 2006-2020 by Andreas Rumpf

git hash: 2220aaeaef74cb6018f4689af8f280db22cb30dd
% g++ -c -w -pthread -O3 -I/mnt/software/n/nim/devel/Nim/lib -I/pbi/flash/cdunn/bb/pbqc/nim-pbqc/src -o /home/UNIXHOME/cdunn/.cache/nim/pbqc_r/@mpbqc.nim.cpp.o /home/UNIXHOME/cdunn/.cache/nim/pbqc_r/@mpbqc.nim.cpp
/home/UNIXHOME/cdunn/.cache/nim/pbqc_r/@mpbqc.nim.cpp: In function 'void dispatchversion__U8rNBav1APtLmPefpp58TA(tySequence__sM4lkSb7zS6F7OVMvW9cffQ*, NimStringDesc*, NimStringDesc*, bool, bool, bool, void*)':
/home/UNIXHOME/cdunn/.cache/nim/pbqc_r/@mpbqc.nim.cpp:1439:64: error: cannot declare pointer to 'struct tySequence__00YDaSWHmNq5UYjMPWr9czw*&'
  (*colonenv_).setByP3 = ((tySequence__00YDaSWHmNq5UYjMPWr9czw*&*) NIM_NIL);
                                                                ^
/home/UNIXHOME/cdunn/.cache/nim/pbqc_r/@mpbqc.nim.cpp: In function 'void cligenScope__xEqSq4LMqH8MnQhhbis6TA(tyObject_ClCfg__X9cAGpPXlXCuhsUscsCeZnQ*)':
/home/UNIXHOME/cdunn/.cache/nim/pbqc_r/@mpbqc.nim.cpp:1719:60: error: cannot declare pointer to 'struct tySequence__sM4lkSb7zS6F7OVMvW9cffQ*&'
   if (!!((((void*) (((tySequence__sM4lkSb7zS6F7OVMvW9cffQ*&*) NIM_NIL))) == NIM_NIL))) goto LA3_;
                                                            ^
import cligen #1.4.0
proc version() =
    echo "1.2.3"
when isMainModule:
    dispatch(version)
c-blake commented 3 years ago

To the best of my knowledge, this should work, but has also been broken for months. Basically, I just use a nil default for a ptr var seq[Foo] type in the dispatchGen macro. If that is a Nim language error and not a codegen bug then it should also be rejected or the C backend.

It also fails (slightly differently) with just ptr seq[Foo] which also works with the C backend in say test/SetByParse.nim. (The ptr var vs ptr part is really just "documentation". The code will be doing add on the deref..So, it really needs to be a proper var seq, not some unsafeAddr thing or some other source of ptr.)

I have not tried to find a minimal reproducer or report this, though I think since it's been there a while someone should tell Nim core. Just doing some tiny new macro with a similar calling convention would be a good place to start.

pb-cdunn commented 3 years ago

It definitely works with c backend. I've posted a minimal example, but I've also verified that it also fails when version() returns int.

Yes, it's probably a Nim bug, but it's frustrating.

c-blake commented 3 years ago

Oh, every single test prog fails with cpp backend lately..version, no versions, etc. I should have also mentioned that just before the setByParse parameter is a similar docs parameter.

I am 99% sure this is not a cligen-specific issue. There would have to be a move afoot in Nim-core to forbid nil-defaulted ptr macro parameters for this to be something I would strictly have to act upon in this repository. My blind guess would be that some kind of strict non-nil mode is assumed in parts of the cpp codegen where it should not be. Maybe someday nil will go away completely? I don't know how firm those plans are. But as long as it is a fine value for == and returns, I think it should be a fine value for parameter defaults.

That said, I may be able to workaround the bug for you. nil here is just a convenient user-passed-nothing sentinel { more or less what it always is... ;-) }. I just tested that things compile and seem to work ok if instead of nil I use the addr of dummy variables of types var seq[string] and var seq[ClParse] for those default values. If we want to report this to Nim core and they then fix things in a reasonable time frame, then I should maybe hold off doing that, though.

c-blake commented 3 years ago

I should also have mentioned that this works on nim-1.4. Only nim-devel versions fail, but presumably there are some other nuts & bolts that pop off if you do not use nim-devel. I have a fix in the works that should be backward compatible to at least nim-0.19.2 (as most of cligen is). Stay tuned.

c-blake commented 3 years ago

Well, it should all work fine in cligen-devel. Let me know if you need me to punch a release.

pb-cdunn commented 3 years ago

Thank you! Now if I can only convince people at work to let me wrap their C++ code with Nim.

c-blake commented 3 years ago

Lol. Well, good luck. 2021 is supposed to be a year of polishing/stability work. Here's hoping.