revert #13064

[krux02]

#13064 should not have passed quality control.

issues with lenTuple:

• The name should be tupleLen, because a length tuple is a tuple containing dimensions like with height and depath.
• The implementation returns the length of the AST node. That is only in rare cases the actual length of a tuple.

issues with get:

• The name get is too universal. It is very likely to clash with symbols of other libraries that also have a get proc.

issues with genericParams:

• genericParams returns a tuple of typedesc values. This is exactly the type of typedesc usage that we agreed to drop from the language. Same with arrays of typedesc.

Edit:

from my experience with macros I can tell, that you need for every typetrait proc a variant that takes NimNode, because that is the way you get types within macros, both in the argument form as well as in the result of getTypeInst. It is not possible to call the procs operating on typedesc from a macro. But if you have the NimNode proc implemented, it is trivial to infer a version that works on typedesc with a macro. It is also nice if the macros api and the typetraits api have matching names.

[Clyybber]

This shouldn't be removed IMO. The other stuff I agree should be removed

[krux02]

I think it should be removed. The informal reason is: shitty typedesc. There is even a PR that I am currently working on that would make this solution impossible #11959

[Clyybber]

Can you point out whats bad about it here? (sorry for disrupting your crusade :p)
I think preventing to return typedesc is a very bad idea. I can understand it for procs, but not for templates/macros.

[krux02]

nothing against returning typedesc from a macro. But putting typedesc in any form of container, such as array or tuple is bad. What do you think (int,float) is? Is it the type for a tuple of int and float, or is it a tuple containing the type int and the type float? If you print the result of this proc you will get somthing that will be something different when parsed. There are probably more problems under the hood that I can't name, because typedesc always has hidden problems.

[krux02]

I just found out that this function does return a tuple type, not a tuple value. And and since a tuple type does not have accessors, there accessors were added to typetraits.

[genotrance]

@nitely has styleCheck set to error for nim-regex which is causing this failure.

https://github.com/nitely/nim-regex/blob/master/config.nims#L4

[timotheecour]

The name should be tupleLen, because a length tuple is a tuple containing dimensions like with height and depath.

that was consistency with lenVarargs. If you want to change lenTuple => tupleLen, you may also want to change lenVarargs => varargsLen.

The implementation returns the length of the AST node. That is only in rare cases the actual length of a tuple.

indeed, and good point. But that should be fixed instead of reverted. I can look into it.

The name get is too universal. It is very likely to clash with symbols of other libraries that also have a get proc.

feel free to suggest a better name

genericParams

I don't see what you'd want to revert that. If it's broken please show a failing example.

[krux02]

I don't see what you'd want to revert that. If it's broken please show a failing example.

The problem is, the entire pull request should be reverted and set back to the drawing board. A fresh start of the idea. As I said earlier this PR should not be accepted in the first place. For genericParams it is again the name that is misleading. If you read genericParams you would think that you would get a list of parameters or parameter types. But that is not the case. The procedure does a type conversion and returns a tuple type that contains all generic parameters. Then you realized that you can't access the members of tuples either and you introduced the accessors get and lenTuple. I am sorry, but in my eyes this is just screams bad design. What you should have done is to introduce just these two names:

macro lenGenericParams(arg: typedesc): int and macro getGenericParam(arg: typedesc, idx: static int): untyped.

Both lenGenericParams and getGenericParam can then be generalized to work on tuple types as well.

And regarding your request to fix your implementation. No I don't want to do that. I don't want to fix "features" that should not have been introduced in the first place.

[timotheecour]

I am sorry, but in my eyes this is just screams bad design. What you should have done is to introduce just these two names:

that was my 1st approach #8554 but that "bad design" you're referring to was actually suggested by @zah, see: #8554 (comment)

And I agree with him, returning a type tuple results in a better, more reusable API.

And regarding your request to fix your implementation. No I don't want to do that. I don't want to fix "features" that should not have been introduced in the first place.

Like I said here #13401 (comment), I will fix lenTuple with a followup PR.

[krux02]

that was my 1st approach #8554 but that "bad design" you're referring to was actually suggested by @zah, see: #8554 (comment)

And I agree with him, returning a type tuple results in a better, more reusable API.

Well, I disagree. The concept to have typedesc as values is dead, and it caused a lot of friction and trouble in the compiler.

Like I said here #13401 (comment), I will fix lenTuple with a followup PR.

Here are some types that you should test:

type
  VectorElementType = SomeNumber | bool
  Vec*[N : static[int], T: VectorElementType] = object
    arr*: array[N, T]
  Vec4*[T: VectorElementType] = Vec[4,T]
  Vec4f* = Vec4[float32]

  MyTupleType = (int,float,string)
  MyGenericTuple[T] = (T,int,float)
  MyGenericAlias = MyGenericTuple[string]
  MyGenericTuple2[T,U] = (T,U,string)
  MyGenericTuple2Alias[T] =  MyGenericTuple2[T,int]
  MyGenericTuple2Alias2 =   MyGenericTuple2Alias[float]

[timotheecour]

• I'm fixing lenTuple in fix incorrect lenTuple implementation #13423

Here are some types that you should test:

I added those and a few others in #13423; that PR just fixes the bug in lenTuple but doesn't change its name

• fine with me if you want to change the name in your PR to tupleLen + varargsLen after fix incorrect lenTuple implementation #13423 is merged.

• also fine with me if you want to suggest another name instead of get in template get*(T: typedesc[tuple], i: static int): untyped

• As for the other changes you are suggesting (removing genericParams or changing to lenGenericParams, getGenericParam), I don't see it as an improvement. What we have right now is simple and unless you can show an example where it doesn't work I don't see the need to change it:

doAssert genericParams(Foo[float, string]) is (float, string)

• EDIT: I'm also making genericParams work with generics containing static params (eg static[int]) in make genericParams support static[T] generic params #13433; and I've included all the types you mentioned (plus other tests) in that PR

[timotheecour]

/cc @krux02 not related to this PR but FYI: tensordslnim/tests/tests.nim test seems like it's ignoring test failures: adding any doAssert false inside test("sumTransposeLoopCode"): for example doesn't actually make the tested command in important_packages.nim fail (nim c -r tests/tests.nim would show a [FAIL] message but eventually succeed). You may want to use std/unittest or addQuitProc or any other logic to ensure correctness of macro test

I couldn't post an issue on https://krux02@bitbucket.org/krux02/tensordslnim/jira/ since that page is unrepsonsive.

[krux02]

@timotheecour Thanks for the info. For some reason I never implemented exist status propagation because it wasn't needed for local testing. When the tensordsl package moved to important packages, I simply forgot that part. Thank your for the notification. It is fixed now.

[timotheecour]

now that #13433 and #13423 have been merged, IMO the only thing left is renaming lenTuple to tupleLen and lenVarargs to varargsLen.
myTuple.get(0) is fine IMO and in the unlikely even of a clash after overload resolution (unlikely because 1st arg has to be a tuple, I don't see much value to such a proc, and also this didn't cause any failure wrt important_packages), import typetraits except get is good enough of a workaround.

[timotheecour]

and with #13639 that just got merged, I think nothing is left to do here; should this be closed?