tqchen commented on pull request #6319: URL: https://github.com/apache/incubator-tvm/pull/6319#issuecomment-678336322
Thanks @wrongtest . This might revealed another problem, as the default data structure for ndim should be int32_t as in https://github.com/dmlc/dlpack/blob/master/include/dlpack/dlpack.h#L136 So perhaps we want to change the API to use the new value. There is always a temptation to use unsigned integers to represent values that can are non-negative. And there can be an active debate. On one hand, having unsigned values are great because we don't need to worry about negative checks like this one. On the other hand, unsigned values also creates potential problems, when there are a lot of implicit conversion between unsigned and signed, and potential underflow during subtraction. So gradually my view has changed from use unsigned for non-neg values to use signed when possible. To make things consistent, perhaps we should change the arguments of these APIs to int or int32_t. cc @liangfu @areusch ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
