Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix clang format not working in CI #2790

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Fix clang format not working in CI #2790

wants to merge 10 commits into from

Conversation

soheilshahrouz
Copy link
Contributor

@soheilshahrouz soheilshahrouz commented Oct 28, 2024 •
edited
Loading

Solves issue 2667

@github-actions github-actions bot added infra Project Infrastructure lang-shell Shell scripts (bash etc.) labels Oct 28, 2024
@github-actions github-actions bot added VPR VPR FPGA Placement & Routing Tool Odin Odin II Logic Synthesis Tool: Unsorted item libarchfpga Library for handling FPGA Architecture descriptions lang-cpp C/C++ code libvtrutil labels Oct 28, 2024
@soheilshahrouz soheilshahrouz changed the title [WIP] Clang format Clang format Oct 28, 2024
@soheilshahrouz soheilshahrouz changed the title Clang format Fix clang format not working in CI Oct 28, 2024
@github-actions github-actions bot added the lang-python Python code label Oct 28, 2024
soheilshahrouz
soheilshahrouz commented Oct 28, 2024
View reviewed changes
vpr/src/base/check_netlist.cpp
@@ -48,20 +48,19 @@ void check_netlist(int verbosity) {
/* Check that nets fanout and have a driver. */
int global_to_non_global_connection_count = 0;
for (auto net_id : cluster_ctx.clb_nlist.nets()) {
h_net_ptr = insert_in_hash_table(net_hash_table, cluster_ctx.clb_nlist.net_name(net_id).c_str(), size_t(net_id));
h_net_ptr
= insert_in_hash_table(net_hash_table, cluster_ctx.clb_nlist.net_name(net_id).c_str(), size_t(net_id));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

BreakBeforeBinaryOperators: All

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugly. Prefer the BreakBeforeBinaryOperators: NonAssignment to keep the = on the first line.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am indifferent on this one. My personal preference for this line would be:

h_net_ptr = insert_in_hash_table(net_hash_table, 
                                 cluster_ctx.clb_nlist.net_name(...),
                                 ...)

Or the line above should use more temporary variables to make this look nicer (which is good practice on the programmer's part).

soheilshahrouz
soheilshahrouz commented Oct 28, 2024
View reviewed changes
vpr/src/pack/cluster_util.h
enum e_gain_update { GAIN, NO_GAIN };
enum e_feasibility { FEASIBLE, INFEASIBLE };
enum e_gain_type { HILL_CLIMBING, NOT_HILL_CLIMBING };
enum e_removal_policy { REMOVE_CLUSTERED, LEAVE_CLUSTERED };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add AllowShortEnumsOnASingleLine: false

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong preference from me on this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am also indifferent on this one. I actually think its nice; but I do understand if we want consistency.

@soheilshahrouz
Copy link
Contributor Author

soheilshahrouz commented Oct 28, 2024 •
edited
Loading

I changed ColumnLimit from 0 (unlimited) to 120. I suggest a few other changes in .clang-format file. Let me know whay your thought are. For each suggestion, I added a few comments to show how it changes the code.

  • Add AllowShortEnumsOnASingleLine: false
  • AllowShortIfStatementsOnASingleLine should be set to false.
  • BreakBeforeBinaryOperators should be set to NonAssignment.

@vaughnbetz
Copy link
Contributor

Adding @AlexandreSinger and @duck2 in case they want to weigh in.

@vaughnbetz
Copy link
Contributor

Can merge when we get agreement on these few questions ... thanks for driving this!

@AlexandreSinger
Copy link
Contributor

Hi @soheilshahrouz , thank you so much for fixing this! I cannot believe the problem was just a submodule issue. I worry that this PR will trample all PRs currently in flight (it will cause conflicts with all code); but I think it is so worth it.

I was looking at how it reformatted my AP code (because my coding style is perfect 😁) and beyond the if statement to one line thing (which I mentioned above as bad), I agree with most of the changes.

I do have a couple of comments:

  • May I request that we use a line width of 80 instead of 120? I know that its a bit of a meme that I like to follow the LLVM style guide, but I agree with their reasoning as to why to choose 80: https://llvm.org/docs/CodingStandards.html#source-code-width . This is also in agreement with the Google C++ style-guide as well: https://google.github.io/styleguide/cppguide.html#Line_Length . However, I will not kill for this. If the people want 120, so be it. I am just from the class of people that believe that well-written code should never be longer than 80 columns (if it must be, then you can refactor your code better; i.e. more functions and local variables for readability). The issue for me is that I prefer 80 characters, and when I write for 80 characters, this clang format will "fix" it to 120...
  • May I request that we add "InsertNewlineAtEOF: true"? This adds new lines to the end of files. The reason you would want this is that git sometimes gets confused when you modify files in a commit and then undo your modifications. It then "pretends" to delete the last line that does not exist and causes random git conflicts. I strongly believe that all files should end with a new line character to prevent this.
  • I also noticed that Odin-II is being tracked by the clang format, was this intentional? Why are we ensuring that Odin-II follows this style guide? If we do not care about it, we should prevent changing it.

Sorry for the long response, I am very passionate about this!

@AlexandreSinger
Copy link
Contributor

Don't let my message above discourage you Soheil though, I really really really like this change and am excited to get this merged in! I like it when code is consistent throughout the entire repo!

@vaughnbetz
Copy link
Contributor

I think 80 characters is too wrenching; we have a lot of long variable names (which I generally like), and made the decision long ago to abandon 80 characters (partially pushed by Google actually, so I don't think 80 characters is a universal rule there). I think if we went to 80 characters it would be a big make work project.

I think anything controversial should be run by the Thursday meeting too, and an 80 character limit would be in that category in my opinion.

@AlexandreSinger
Copy link
Contributor

@vaughnbetz Understood! I do agree this is something to bring up in the weekly meeting; however, this should not block this PR from getting merged in. As you say, the code is already quite wide, so reducing it now will be just as complicated as reducing it later.

Push come to shove you can change small portions of the clang style in sub-projects (but I do not think the fact that I "want it to be 80" is enough argument to not follow the VTR style guide).

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

@AlexandreSinger AlexandreSinger AlexandreSinger left review comments

@vaughnbetz vaughnbetz vaughnbetz left review comments

Assignees
No one assigned
Labels
infra Project Infrastructure lang-cpp C/C++ code lang-python Python code lang-shell Shell scripts (bash etc.) libarchfpga Library for handling FPGA Architecture descriptions liblog libpugiutil libvtrutil Odin Odin II Logic Synthesis Tool: Unsorted item VPR VPR FPGA Placement & Routing Tool
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@soheilshahrouz @vaughnbetz @AlexandreSinger