Skip to content

[Code Style Change] - Changed Vulkan code to match TVM conventions.#10101

Closed
Raghav-Chakravarthy wants to merge 2 commits intoapache:mainfrom
Raghav-Chakravarthy:main
Closed

[Code Style Change] - Changed Vulkan code to match TVM conventions.#10101
Raghav-Chakravarthy wants to merge 2 commits intoapache:mainfrom
Raghav-Chakravarthy:main

Conversation

@Raghav-Chakravarthy
Copy link
Contributor

Code Style Change - Changed Vulkan code to match TVM conventions.

@masahi
Copy link
Member

masahi commented Jan 29, 2022

Thanks for the suggestion, I don't see a need to apply this change. If @Lunderberg likes this patch we can reopen.

@masahi masahi closed this Jan 29, 2022
@Lunderberg
Copy link
Contributor

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.

@Lunderberg
Copy link
Contributor

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.

@Mousius
Copy link
Member

Mousius commented Jan 31, 2022

@Lunderberg wouldn't it be great if we worked towards being able to do that? 😸 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?

@Lunderberg
Copy link
Contributor

@Lunderberg wouldn't it be great if we worked towards being able to do that? 😸 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:

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants