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.

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

CODE: Use custom LLVM build, improve clang-format configuration #6013

Merged
merged 1 commit into from
Dec 29, 2020

Conversation

gleon99
Copy link
Contributor

@gleon99 gleon99 commented Dec 10, 2020

What

Use a custom build of clang-format when formatting code in the pipeline, which fixes pointer alignment and supports alignment of consecutive declarations even if there is a comment between them.

Why ?

Improve the automatic code formatting done in the CI, by making it consistent and closer to our coding style, so that making it a mandatory step would make sense.

buildlib/fedora.Dockerfile Outdated Show resolved Hide resolved
buildlib/fedora.Dockerfile Outdated Show resolved Hide resolved
buildlib/fedora.Dockerfile Outdated Show resolved Hide resolved
.clang-format Show resolved Hide resolved
buildlib/az-helpers.sh Show resolved Hide resolved
@gleon99
Copy link
Contributor Author

gleon99 commented Dec 14, 2020

bot:pipe:retest

buildlib/fedora.Dockerfile Outdated Show resolved Hide resolved
Copy link
Contributor

@yosefe yosefe left a comment

Choose a reason for hiding this comment

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

can we also disable SortIncludes for now?
till we figure out a good method for IncludeCategories

@yosefe
Copy link
Contributor

yosefe commented Dec 14, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines failed to run 1 pipeline(s).

@yosefe
Copy link
Contributor

yosefe commented Dec 15, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines failed to run 1 pipeline(s).

@yosefe
Copy link
Contributor

yosefe commented Dec 15, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@gleon99
Copy link
Contributor Author

gleon99 commented Dec 16, 2020

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 6013 in repo openucx/ucx

@brminich
Copy link
Contributor

issue is #6011

bot:pipe:retest

Copy link
Contributor

@yosefe yosefe left a comment

Choose a reason for hiding this comment

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

can you pls add bad C file for example for format fail?

@gleon99
Copy link
Contributor Author

gleon99 commented Dec 16, 2020

@yosefe , added format error in bitmap.h (assuming we're reverting it before merge).

@yosefe
Copy link
Contributor

yosefe commented Dec 16, 2020

@gleon99 looks like some weird error + job does not show "success with issues"

2020-12-16T08:37:08.9026249Z [command]/usr/bin/bash --noprofile --norc /__w/_temp/49792ab4-09cd-43a4-b293-b11ed3cec313.sh
2020-12-16T08:37:08.9081875Z Checking code format on diff remotes/origin/master..2d3be2ec4d604d84a88d65e17760cb1348ae48d0
2020-12-16T08:37:08.9888945Z Configuration file(s) do(es) not support C++: /__w/1/s/.clang-format
2020-12-16T08:37:08.9897580Z error: `clang-format -lines=193:193 -assume-filename=src/ucs/datastruct/bitmap.h` failed
2020-12-16T08:37:08.9997926Z Generated patch file:
2020-12-16T08:37:09.0042350Z error: unrecognized input
2020-12-16T08:37:09.0627586Z ##[section]Finishing: Bash

@gleon99
Copy link
Contributor Author

gleon99 commented Dec 16, 2020

@yosefe , hopefully fixed. I think master is currently broken (the .clang-format file). A --- separator has to be put between language sections.

@yosefe
Copy link
Contributor

yosefe commented Dec 16, 2020

@gleon99 need to push merge commit with master and then push this fix.. or squash and rebase everything

@gleon99
Copy link
Contributor Author

gleon99 commented Dec 16, 2020

Rebased over master & squashed everything. Don't merge yet, the incorrectly formatted file is still there.

@yosefe
Copy link
Contributor

yosefe commented Dec 17, 2020

few things are still off.
maybe the patch for clang is mis-applied?

--- a/test/apps/test_cuda_hook.c
+++ b/test/apps/test_cuda_hook.c
@@ -15,7 +15,7 @@
 
 static void event_cb(ucm_event_type_t event_type, ucm_event_t *event, void *arg)
 {
-    int *count_p = arg;
+    int        *count_p = arg;
     const char *title;
 
     if (event_type == UCM_EVENT_MEM_TYPE_ALLOC) {
@@ -37,11 +37,11 @@ int main(int argc, char **argv)
 {
     static const ucm_event_type_t memtype_events = UCM_EVENT_MEM_TYPE_ALLOC |
                                                    UCM_EVENT_MEM_TYPE_FREE;
-    static const int num_expected_events         = 2;
-    ucs_status_t status;
-    ucp_context_h context;
-    ucp_params_t params;
-    int num_events;
+    static const int num_expected_events = 2;
+    ucs_status_t     status;
+    ucp_context_h    context;
+    ucp_params_t     params;
+    int              num_events;
  1. SortIncludes is true for Cpp (should be false)

@@ -1044,16 +1044,17 @@ UCS_TEST_SKIP_COND_F(malloc_hook, bistro_patch, RUNNING_ON_VALGRIND) {
 
     /* set hook to mmap call */
     {
-        bistro_patch patch(symbol, (void*)bistro_hook<0>::munmap);
+        bistro_patch patch(symbol, (void *)bistro_hook<0>::munmap);
 
-        munmap_f = (munmap_f_t*)ucm_bistro_restore_addr(rp);
+        munmap_f = (munmap_f_t *)ucm_bistro_restore_addr(rp);
         EXPECT_NE((intptr_t)munmap_f, 0);
  1. Need to set:
BraceWrapping:
  AfterClass: false

@gleon99
Copy link
Contributor Author

gleon99 commented Dec 27, 2020

@yosefe ,
(1) - That's the correct and expected behavior when aligning declarations.

(2) - The patch doesn't deal with that case. The thing there is the

static const ucm_event_type_t memtype_events = UCM_EVENT_MEM_TYPE_ALLOC |
                                                    UCM_EVENT_MEM_TYPE_FREE;

line which breaks the assignment alignment. If it was a single line i.e

static const ucm_event_type_t memtype_events = UCM_EVENT_MEM_TYPE_ALLOC;

instead, everything would work fine (checked). Do we want to fix that in the patch?

(3) - Fixed.

(4) - That's legitimate behaviour as well (PointerAlignment: Right), unless we decide to fix it too.

(5) - Fixed.

@yosefe
Copy link
Contributor

yosefe commented Dec 27, 2020

@gleon99 ok, we can improve it more later on. pls sqaush and remove the badly formatted file and then we can merge it

@gleon99
Copy link
Contributor Author

gleon99 commented Dec 27, 2020

@yosefe , removed the misformatted file and squashed.

@yosefe
Copy link
Contributor

yosefe commented Dec 28, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yosefe
Copy link
Contributor

yosefe commented Dec 29, 2020

bot:pipe:retest

@yosefe yosefe merged commit 7edc0f5 into openucx:master Dec 29, 2020
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