Linting as per google guide #241
Conversation
* refactor * refactor * revert * refactor: clang format * Update icicle/appUtils/msm/msm.cu
There was a problem hiding this comment.
this is basically continuation of this discussion still echoing past these two days 😃 as initially, I hoped we could stick with two naming conventions snake_case (mainly due to Rust being defacto standard) for our code and camelCase for CUDA that we can't change anyway 🥲 . Now if we pick Google's (just because of gtest?) - we'll have to keep up with three ie PascalCase and one is a bit puzzling imho I'm just worried we will have to adhere to one that was neither initial choice?, nor one that allows less options(camelCase and following CUDA seems more reasonable). And we can easily justify snake_case cause it's quite common (in Linux sources) and we won't be alone - since seems like our fellow friends-competitors also use it
lets merge the PR since it's anyway wip and I already did merged it to my wip:) but seems it needs discussion - as I'm not neutral side here and seems like the majority doesn't care too much? 😃
| @@ -10,21 +14,21 @@ namespace { | |||
| #define MAX_THREADS_PER_BLOCK 256 | |||
|
|
|||
| template <typename E, typename S> | |||
| __global__ void mul_kernel(S* scalar_vec, E* element_vec, size_t n, E* result) | |||
| __global__ void mul_kernel(S* scalar_vec, E* element_vec, int n, E* result) | |||
There was a problem hiding this comment.
oh - probably one thing truly important here - umm replacing unsigned types with signed int - feels like a potential cause of issues - ie due to shift operations etc. Also, distinguishing indexes from sizes? with size_t seems more rational than not
There was a problem hiding this comment.
icicle/icicle/appUtils/msm/msm.cu(958): warning #2361-D: invalid narrowing conversion from "unsigned long" to "int"
msm_size,
^
There was a problem hiding this comment.
You're right about narrowing conversions ofc, I'll fix it, just haven't finished re-working internals. https://google.github.io/styleguide/cppguide.html#Integer_Types says to "Use plain old int for such things (as loop counters)". I think loop counters/indices and sizes should be the same type? After all, loop counters range from 0 to size, so making their types different is weird.
There was a problem hiding this comment.
Also about narrowing conversions: I can't reproduce the warning even with -Wconversion flag set for compilation. Also there's no line 958 in msm.cu file, maybe you're looking at some older version?
Yea, we should have a discussion about this. Its interesting to see matter-labs using snake case since their clang is based on LLVM which says to use camelCase. I don't think we should choose a style for the core (c++) based on other languages that we have bindings for (Golang uses camelCase and PascalCase, python uses snake_case, etc...) Personally, I'd vote camelCase but i'd rather a quick decision so if someone feels strongly about another style, I'm fine with it |
One argument for PascalCase for our codebase that I see is what happens after we add prefixes during build. If PascalCase is used, we can get PascalCase or camelCase depending on our preferences, while for camelCase we'll get mixed case as a result (for instance, |
Changes to make our codebase closer to https://google.github.io/styleguide/cppguide.html. Mostly concerning public interfaces. Some fixes discussed offline with @vhnatyk, fixed msm test in CUDA.