I have experienced that a considerable number of review cycles are spent fixing code-style issues that escapes the current linter. E.g., pointer formatting – int* var vs int *var. Moreover, we have a .clang-format in TVM. I was wondering, Could there be some work done incoporating these two to enforce the code-style in the linter itself? I would like to hear your opinions on this.
A related PR with more discussion
@tqchen Is that possible for you to run clang-format for an entire code base so that we can add a checker to CI? If we have concerns to the correctness issues potentially could be introduced by clang-format, we might be able to assign few people to do so?
1 Like
Let us open the discussion open for a bit, if we agree that it is a good idea, we can carry it out during a weekend(when there is less on-going PRs).
There was a related issue https://github.com/apache/incubator-tvm/issues/1732 that resulted in the introduction of .clang-format file. But we didn’t do anything else to improve consistency of existing code or enforce the style for future code.
To be clear, we are enforcing the Google C style via cpplint. Although the cpplint checks seems to be weaker than the clang-format
Maybe take the next steps ?
- Do a flag day clang-format rewrite and take the one time cost for every patch having a merge conflict ?
- Once we are clang-format clean, we could have CI run clang-format and fail CI instantly if there is any change in the source base compared to the pull request ? At the same time we do this we recommend that developers use clang-format in their editors as part of the coding guidelines ? The documentation seems pretty comprehensive and the integration seems straight forward with quite a range of editors / IDEs.
regards Ramana
I will investigate around the next two available weekends(when impact to the CI will be lower)
Would clang-format replace cpplint, or would we have both?
I think the cpplint is quite fast, so it won’t hurt to have both, as long as they do not conflict
Thank you for the proposal! I think it is a great idea to have clang-format in the CI system to enforce coding styles. A quick question, shall we consider clang-tidy some day in the future?
clang-tidy certainly looks interesting as well as something deeper than clang-format and that is likely to help us with other aspects that we may be missing. However I’m probably a bit old-school and would probably be a bit more careful about clang-tidy -fix … 
That might be the next step, though we may have to create a configuration file if there exists something like that for clang-tidy which is a derivative of the google coding style with any minor deviations we make.
1 Like