Skip to content

Conversation

@Raghav-Chakravarthy
Copy link
Contributor

[Issue]
While reviewing the tvm code, I noticed some naming convention issues
in the diag_ctx_ and current_func variables.

Variable current_func should be current_func_ because it is a class
variable

Variable diag_ctx_ should be diag_ctx , because it is a public variable

[Solution]

Changed the variables to match the tvm code style conventions

@Raghav-Chakravarthy Raghav-Chakravarthy changed the title [Code Style Change] Changed code to match the tvm code style conventions. [Code Style] Changed code to match the tvm code style conventions. Sep 18, 2021
[Issue]
While reviewing the tvm code, I noticed some naming convention issues
in the diag_ctx_ and current_func variables.

Variable current_func should be current_func_ because it is a class
variable

Variable diag_ctx_ should be diag_ctx , because it is a public variable

[Solution]

Changed the variables to match the tvm code style conventions
Copy link
Contributor

@mbrookhart mbrookhart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

You have a lint issue, could you run make format and repush?

@Mousius
Copy link
Member

Mousius commented Sep 21, 2021

Hi @mbrookhart / @Raghav-Chakravarthy,

I actually made this mistake in another PR (#8951 (comment)), the actual rule is that all data members of a class should have the trailing _ (https://google.github.io/styleguide/cppguide.html#Variable_Names) and should actually all be private with accessor functions if necessary (https://google.github.io/styleguide/cppguide.html#Access_Control).

@Raghav-Chakravarthy would it be possible for you to update this PR accordingly?

@Raghav-Chakravarthy
Copy link
Contributor Author

@mbrookhart @Mousius

Sorry for the late response. I was busy with many school exams, and got a chance to look at the code this week.

All review comments have been addressed. Please let me know if anything else needs to be modified.

@areusch
Copy link
Contributor

areusch commented Oct 12, 2021

@mbrookhart @Mousius could you take a look at this one? @Raghav-Chakravarthy could you fix the lint error?

Copy link
Member

@Mousius Mousius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the minor lint grievance is resolved, this is a great example of where following the conventions has lead to the code being cleaner and generally easier to read - thanks for this @Raghav-Chakravarthy!

@areusch
Copy link
Contributor

areusch commented Oct 14, 2021

@Raghav-Chakravarthy please fix the lint error when you get a minute!

@Raghav-Chakravarthy
Copy link
Contributor Author

@Mousius @areusch

Sorry for the delayed response.

I will fix the error as soon as possible.

@Raghav-Chakravarthy
Copy link
Contributor Author

@Mousius @areusch
Thank you, I have fixed the linting errors.

@Raghav-Chakravarthy
Copy link
Contributor Author

@areusch @mbrookhart - Please take a look at my PR.

@masahi masahi merged commit e830a1f into apache:main Oct 22, 2021
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 7, 2022
…pache#9040)

* [Code Style] Changed code to match the tvm code style conventions.
[Issue]
While reviewing the tvm code, I noticed some naming convention issues
in the diag_ctx_ and current_func variables.

Variable current_func should be current_func_ because it is a class
variable

Variable diag_ctx_ should be diag_ctx , because it is a public variable

[Solution]

Changed the variables to match the tvm code style conventions

* addressed comments

* removed debug logic

* fixed plint issue

* fixed building issue

* fixed whitespace issue

* fixed linting error in type_solver.cc
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
…pache#9040)

* [Code Style] Changed code to match the tvm code style conventions.
[Issue]
While reviewing the tvm code, I noticed some naming convention issues
in the diag_ctx_ and current_func variables.

Variable current_func should be current_func_ because it is a class
variable

Variable diag_ctx_ should be diag_ctx , because it is a public variable

[Solution]

Changed the variables to match the tvm code style conventions

* addressed comments

* removed debug logic

* fixed plint issue

* fixed building issue

* fixed whitespace issue

* fixed linting error in type_solver.cc
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.

5 participants