Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

BUILD: update clang format #1192

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

Open
Sergei-Lebedev wants to merge 2 commits into openucx:master
base: master
Choose a base branch
Loading
from Sergei-Lebedev:topic/update_clang_format

Conversation

Copy link
Contributor

@Sergei-Lebedev Sergei-Lebedev commented Oct 6, 2025

What

Update clang format

Why ?

Make formatting stable

How ?

Change function wrap:
When function declaration cannot be placed on a single line then place all arguments on a second line

/* before */
static inline void
ucc_knomial_pattern_init(ucc_rank_t size, ucc_rank_t rank, ucc_kn_radix_t radix,
 ucc_knomial_pattern_t *p)
{
 ucc_knomial_pattern_init_impl(size, rank, radix, p, 0, 1);
}
/* after */
static inline void ucc_knomial_pattern_init(
 ucc_rank_t size, ucc_rank_t rank, ucc_kn_radix_t radix,
 ucc_knomial_pattern_t *p)
{
 ucc_knomial_pattern_init_impl(size, rank, radix, p, 0, 1);
}

When function call cannot be placed on a single line then place one arg per line

/* before */
 status =
 ucc_mc_memcpy(rbuf, scratch_header->addr, tsize * data_size,
 rmem, UCC_MEMORY_TYPE_HOST);
 if (ucc_unlikely(status != UCC_OK)) {
 tl_error(UCC_TASK_LIB(task),
 "failed to copy from scratch buffer to dst");
 task->super.status = status;
 return;
 }
/* after */
 status = ucc_mc_memcpy(
 rbuf,
 scratch_header->addr,
 tsize * data_size,
 rmem,
 UCC_MEMORY_TYPE_HOST);
 if (ucc_unlikely(status != UCC_OK)) {
 tl_error(
 UCC_TASK_LIB(task),
 "failed to copy from scratch buffer to dst");
 task->super.status = status;
 return;
 }

reflow comments

/* before */
/**
 * Computes actual radix at current iteration
 * @param [in] p ucc_knomial_pattern
 * @return radix
 */
static inline
ucc_rank_t ucc_kn_compute_step_radix(ucc_knomial_pattern_t *p);
/* after */
/**
 * Computes actual radix at current iteration
 * @param [in] p ucc_knomial_pattern
 * @return radix
 */
static inline ucc_rank_t ucc_kn_compute_step_radix(ucc_knomial_pattern_t *p)

Please avoid using inline comments as they break alignment. E.g.

/*with inline comments*/
typedef struct ucc_knomial_pattern {
 ucc_kn_radix_t radix; /* knomial tree radix */
 uint8_t type; /* pattern type */
 uint8_t iteration; /* current iteration */
 uint8_t n_iters; /* number of iterations in knomial algorithm */
 uint8_t
 pow_radix_sup; /* smallest integer N such that (radix ** N) >= size */
 uint8_t node_type; /* type of current rank: BASE, PROXY or EXTRA */
 uint8_t backward; /* boolean, iteration direction */
 ucc_rank_t
 radix_pow; /* power of radix for current algorithm iteration
 * forward: initial value is 1
 * backward: initial valus is full_pow_size if have >1 full subtrees, OR
 * (full_pow_size / radix) otherwise
 */
 ucc_rank_t
 full_pow_size; /* largest power of radix <= size. It is equal to
 * (radix ** pow_radix_sup) if (radix ** pow_radix_sup) == size, OR
 * (radix ** (_pow_radix_sup - 1)) otherwise
 */
 ucc_rank_t size; /* total number of ranks */
 ucc_rank_t rank; /* process rank */
 ucc_rank_t n_extra; /* number of "extra" ranks to be served by "proxies" */
 size_t block_size_counts;
 size_t count; /* collective buffer size */
 ucc_count_t *counts;
 ucc_rank_t block_size;
 ptrdiff_t block_offset;
 int is64;
} ucc_knomial_pattern_t;
/* no inline comments */
typedef struct ucc_knomial_pattern {
 /* knomial tree radix */
 ucc_kn_radix_t radix;
 /* pattern type */
 uint8_t type;
 /* current iteration */
 uint8_t iteration;
 /* number of iterations in knomial algorithm */
 uint8_t n_iters;
 /* smallest integer N such that (radix ** N) >= size */
 uint8_t pow_radix_sup;
 /* type of current rank: BASE, PROXY or EXTRA */
 uint8_t node_type;
 /* boolean, iteration direction */
 uint8_t backward;
 /* power of radix for current algorithm iteration
 * forward: initial value is 1
 * backward: initial valus is full_pow_size if have >1 full subtrees, OR
 * (full_pow_size / radix) otherwise
 */
 ucc_rank_t radix_pow;
 /* largest power of radix <= size. It is equal to
 * (radix ** pow_radix_sup) if (radix ** pow_radix_sup) == size, OR
 * (radix ** (_pow_radix_sup - 1)) otherwise
 */
 ucc_rank_t full_pow_size;
 /* total number of ranks */
 ucc_rank_t size;
 /* process rank */
 ucc_rank_t rank;
 /* number of "extra" ranks to be served by "proxies" */
 ucc_rank_t n_extra;
 size_t block_size_counts;
 /* collective buffer size */
 size_t count;
 ucc_count_t *counts;
 ucc_rank_t block_size;
 ptrdiff_t block_offset;
 int is64;
} ucc_knomial_pattern_t;

ikryukov reacted with thumbs up emoji
Copy link
Collaborator

ikryukov commented Oct 6, 2025

what is minimal clang-format version that supports new style? I've checked that 19 is not enough, looks like 21?

Copy link
Contributor Author

what is minimal clang-format version that supports new style? I've checked that 19 is not enough, looks like 21?

I tested with 21.0, but I would recommend to use latest available always. Currently there are still limitations where clangformat cannot keep formatting consistent and I saw some PRs in llvm repo that addresses these issues.

Copy link
Collaborator

ikryukov commented Oct 7, 2025

what is minimal clang-format version that supports new style? I've checked that 19 is not enough, looks like 21?

I tested with 21.0, but I would recommend to use latest available always. Currently there are still limitations where clangformat cannot keep formatting consistent and I saw some PRs in llvm repo that addresses these issues.

then need to update clang format 17 to 21 in CI pipelines, will use 21 for pre-commit

Copy link
Collaborator

@ikryukov ikryukov left a comment

Choose a reason for hiding this comment

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

tested with clang 21 - ok to change

Copy link
Collaborator

@wfaderhold21 wfaderhold21 left a comment

Choose a reason for hiding this comment

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

LGTM

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

Reviewers

@wfaderhold21 wfaderhold21 wfaderhold21 approved these changes

@manjugv manjugv Awaiting requested review from manjugv

@janjust janjust Awaiting requested review from janjust

@nsarka nsarka Awaiting requested review from nsarka

@samnordmann samnordmann Awaiting requested review from samnordmann

+1 more reviewer

@ikryukov ikryukov ikryukov approved these changes

Reviewers whose approvals may not affect merge requirements

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /