[GitHub] [tvm] kparzysz-quic commented on pull request #10014: Fix function number datatype from char to uint16_t

[GitBox <gi...@apache.org>]

kparzysz-quic commented on pull request #10014:
URL: https://github.com/apache/tvm/pull/10014#issuecomment-1019605219


   I agree that accessing C structures via LLVM codegen may be tricky (due to pointer sizes in particular, and alignment rules), we're already running into a similar issue with enums in `Device` (IIRC).  I should be ok to have the number of functions "hidden" in the buffer, but we should have separate functions to (1) read it from the buffer, (2) store it to the buffer, (3) get the beginning of the actual payload.  This is what I meant in my first comment---this way we would avoid things like `for (char *p = names + 1; ...)`, where it's not clear what that `+ 1` means, and instead have something like `for (char *p = some_better_name_for_the_beginning_of_the_actual_names(); ...)`.  This would localize the encoding and decoding of the function count to specialized function, where the situation can be explained in comments.  This is all to make it harder for people to make an accidental mistake.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org