Fix clang format not working in CI #2790

soheilshahrouz · 2024-10-28T15:52:22Z

…runners

libs/libarchfpga/src/device_grid.cpp

libs/libarchfpga/src/histogram.cpp

soheilshahrouz · 2024-10-28T21:10:17Z

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));


BreakBeforeBinaryOperators: All

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

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 · 2024-10-28T21:11:37Z

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 };


Add AllowShortEnumsOnASingleLine: false

No strong preference from me on this one.

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

soheilshahrouz · 2024-10-28T21:11:53Z

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 · 2024-10-28T22:37:09Z

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

vaughnbetz · 2024-10-28T22:38:05Z

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

AlexandreSinger · 2024-10-28T23:54:14Z

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 · 2024-10-29T00:07:43Z

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 · 2024-10-29T03:08:38Z

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 · 2024-10-29T14:51:18Z

@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).

print the output of make format to see why it doesn't work on github …

de464fc

…runners

github-actions bot added infra Project Infrastructure lang-shell Shell scripts (bash etc.) labels Oct 28, 2024

soheilshahrouz added 3 commits October 28, 2024 11:58

init submodules when checking out in Format matrix

9393fbf

update .clang-format

b8a230c

make format

430f080

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 changed the title ~~[WIP] Clang format~~ Clang format Oct 28, 2024

revert changes made for debugging purposes

09108db

soheilshahrouz changed the title ~~Clang format~~ Fix clang format not working in CI Oct 28, 2024

make format-py

fd314f3

github-actions bot added the lang-python Python code label Oct 28, 2024

ColumnLimit: 120 in .clang-format

1a5894a

github-actions bot added liblog libpugiutil labels Oct 28, 2024

soheilshahrouz commented Oct 28, 2024

View reviewed changes

libs/libarchfpga/src/device_grid.cpp Outdated Show resolved Hide resolved

soheilshahrouz commented Oct 28, 2024

View reviewed changes

libs/libarchfpga/src/histogram.cpp Outdated Show resolved Hide resolved

soheilshahrouz commented Oct 28, 2024

View reviewed changes

fix compilation error in write_models_bb.cpp

8d043fd

soheilshahrouz added 2 commits October 28, 2024 19:32

fix format errors

a2d9b0b

set AllowShortIfStatementsOnASingleLine to false

2e6d089

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix clang format not working in CI #2790

Fix clang format not working in CI #2790

soheilshahrouz commented Oct 28, 2024 •

edited

Loading

soheilshahrouz Oct 28, 2024

vaughnbetz Oct 28, 2024

AlexandreSinger Oct 28, 2024

soheilshahrouz Oct 28, 2024

vaughnbetz Oct 28, 2024

AlexandreSinger Oct 28, 2024

soheilshahrouz commented Oct 28, 2024 •

edited

Loading

vaughnbetz commented Oct 28, 2024

vaughnbetz commented Oct 28, 2024

AlexandreSinger commented Oct 28, 2024

AlexandreSinger commented Oct 29, 2024

vaughnbetz commented Oct 29, 2024

AlexandreSinger commented Oct 29, 2024

Fix clang format not working in CI #2790

Are you sure you want to change the base?

Fix clang format not working in CI #2790

Conversation

soheilshahrouz commented Oct 28, 2024 • edited Loading

soheilshahrouz Oct 28, 2024

Choose a reason for hiding this comment

vaughnbetz Oct 28, 2024

Choose a reason for hiding this comment

AlexandreSinger Oct 28, 2024

Choose a reason for hiding this comment

soheilshahrouz Oct 28, 2024

Choose a reason for hiding this comment

vaughnbetz Oct 28, 2024

Choose a reason for hiding this comment

AlexandreSinger Oct 28, 2024

Choose a reason for hiding this comment

soheilshahrouz commented Oct 28, 2024 • edited Loading

vaughnbetz commented Oct 28, 2024

vaughnbetz commented Oct 28, 2024

AlexandreSinger commented Oct 28, 2024

AlexandreSinger commented Oct 29, 2024

vaughnbetz commented Oct 29, 2024

AlexandreSinger commented Oct 29, 2024

soheilshahrouz commented Oct 28, 2024 •

edited

Loading

soheilshahrouz commented Oct 28, 2024 •

edited

Loading