You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/01/28 21:28:45 UTC
[GitHub] [tvm] Raghav-Chakravarthy opened a new pull request #10101: [Code Style Change] - Changed Vulkan code to match TVM conventions.
Raghav-Chakravarthy opened a new pull request #10101:
URL: https://github.com/apache/tvm/pull/10101
Code Style Change - Changed Vulkan code to match TVM conventions.
--
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
[GitHub] [tvm] Lunderberg commented on pull request #10101: [Code Style Change] - Changed Vulkan code to match TVM conventions.
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #10101:
URL: https://github.com/apache/tvm/pull/10101#issuecomment-1060856837
> @Lunderberg wouldn't it be great if we worked towards being able to do that? :smile_cat: Personally I think we should encourage patches like this which improve our coding standards and follow the guidelines we set out for ourselves.
@Mousius Definitely agreed, and I'd like for it to be possible, either with a CI step or a standard pre-commit hook to format any modified files. I haven't had the bandwidth lately, but in case anybody reading this does, the options I used to search were as follows:
* Disable all clang-tidy passes except [readability-identifier-naming](https://clang.llvm.org/extra/clang-tidy/checks/readability-identifier-naming.html) with `-checks=-*,readability-identifier-naming`
* Set the option for [PrivateMemberCase](https://clang.llvm.org/extra/clang-tidy/checks/readability-identifier-naming.html#cmdoption-arg-privatemembercase) and [PublicMemberCase](https://clang.llvm.org/extra/clang-tidy/checks/readability-identifier-naming.html#cmdoption-arg-publicmembercase) to `lower_case`
* Set the option for [PrivateMemberSuffix](https://clang.llvm.org/extra/clang-tidy/checks/readability-identifier-naming.html#cmdoption-arg-privatemembersuffix) to `_`
* Set the option for [PublicMemberSuffix](https://clang.llvm.org/extra/clang-tidy/checks/readability-identifier-naming.html#cmdoption-arg-publicmembersuffix) to an empty string.
> Having said that, the correct approach under the Google C++ Guidelines is all data should be _ suffixed and getters added iirc?
It does, yes. The reasoning is that private member variables with getters/setters can be replaced, while maintaining the same external behavior, but there is no way to replace public member variables without breaking at least some previously-allowed behavior. Essentially, structs to represent data, and classes to represent behavior, with no mixing allowed between them. I like doing so overall, but it would be a very dramatic shift from the existing public member variables.
--
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
[GitHub] [tvm] masahi closed pull request #10101: [Code Style Change] - Changed Vulkan code to match TVM conventions.
Posted by GitBox <gi...@apache.org>.
masahi closed pull request #10101:
URL: https://github.com/apache/tvm/pull/10101
--
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
[GitHub] [tvm] Lunderberg commented on pull request #10101: [Code Style Change] - Changed Vulkan code to match TVM conventions.
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #10101:
URL: https://github.com/apache/tvm/pull/10101#issuecomment-1025977511
Running a quick check with `clang-tidy`, it looks like there's a few hundred similar member variables that don't follow the convention of using an underscore iff the variable is private. From that, I don't think it's worth manually changing the names, unless we want to run `clang-tidy` on everything and add it as a CI step.
--
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
[GitHub] [tvm] masahi commented on pull request #10101: [Code Style Change] - Changed Vulkan code to match TVM conventions.
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #10101:
URL: https://github.com/apache/tvm/pull/10101#issuecomment-1025000269
Thanks for the suggestion, I don't see a need to apply this change. If @Lunderberg likes this patch we can reopen.
--
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
[GitHub] [tvm] Lunderberg commented on pull request #10101: [Code Style Change] - Changed Vulkan code to match TVM conventions.
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on pull request #10101:
URL: https://github.com/apache/tvm/pull/10101#issuecomment-1025975654
I don't have a strong opinion either way. I don't have any issues with updating to the convention of using the trailing underscore for private members and no trailing underscores for public members, but it also isn't a very consistently followed convention at the moment.
--
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
[GitHub] [tvm] Mousius commented on pull request #10101: [Code Style Change] - Changed Vulkan code to match TVM conventions.
Posted by GitBox <gi...@apache.org>.
Mousius commented on pull request #10101:
URL: https://github.com/apache/tvm/pull/10101#issuecomment-1026064310
@Lunderberg wouldn't it be great if we worked towards being able to do that? :smile_cat: Personally I think we should encourage patches like this which improve our coding standards and follow the guidelines we set out for ourselves.
Having said that, the correct approach under the Google C++ Guidelines is all data should be `_` suffixed and getters added iirc?
--
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