DAOS-623 client: allow variable declaration in loop #15428

jxiong · 2024-10-30T01:08:14Z

This PR tries to enable variable declaration in loop from C99 standard. This will not make coding easier, but will also make the code less error-prone by limiting the scope of looping variables.

The Linux kernel adopts C99 for this exact same reason.

NB: I only updated the variable in the for loops. It seems like this file hasn't been changed since new clang-format is used. Maybe I picked a wrong file to start with ;-)

Before requesting gatekeeper:

Two review approvals and any prior change requests have been resolved.
Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
Commit messages follows the guidelines outlined here.
Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

github-actions · 2024-10-30T01:08:32Z

Ticket title is 'Generic ticket for minor code cleanup and improvement'
Status is 'Resolved'
Labels: 'request_for_2.6.2'
https://daosio.atlassian.net/browse/DAOS-623

wangshilong

It might be better to have a ticket number, I thought there is a MISC ticket for such PRs. @jolivier23

jolivier23 · 2024-10-30T19:46:16Z

It might be better to have a ticket number, I thought there is a MISC ticket for such PRs. @jolivier23

there is DAOS-623

jolivier23

it would appear you don't need to change anything in clang-format. It must not care with current settings.

jxiong · 2024-10-30T20:01:02Z

It might be better to have a ticket number, I thought there is a MISC ticket for such PRs. @jolivier23

@wangshilong This PR is just for reference purpose so that I can collect community's feedback. I will certainly add a daos ticket once we decide to move it forward. Thanks for calling it out.

jxiong · 2024-11-07T03:42:27Z

@jolivier23 I don;t think this CI test (continuous-integration/jenkins/pr-head) would pass, I have tried 3 times but no luck. What would be your suggestion for the next step?

ashleypittman · 2024-11-07T09:18:31Z

@jolivier23 I don;t think this CI test (continuous-integration/jenkins/pr-head) would pass, I have tried 3 times but no luck. What would be your suggestion for the next step?

There was a network issue when the build was starting, I've retriggerd it.

daltonbohning · 2024-11-12T22:30:46Z

Ideally, we should get a review from @daos-stack/client-api-owners before merging.

mchaarawi

i do not get the purpose of doing these changes on just this file. this screws up git blame.
we probably have many other files that do not have clang format and i will request we don't do such changes for now unless we make a discussion to address this everywhere.

mchaarawi · 2024-11-12T22:37:39Z

src/client/api/object.c


 		rc = dc_obj_get_grp_size(oh, &grp_size);
 		if (rc)
 			D_GOTO(out, rc);

-		for (i = 0; i < layout->ol_nr; i++) {
+		for (uint32_t i = 0; i < layout->ol_nr; i++) {


this is not our coding style

We don't have much code that looks like this but keeping the scope of variables as small as possible is something that is explicitly stated in our style guide and it's something that's generally considered best-practice across the industry. I know I've fixed at least one bug in our codebase where a loop variable was misused later in the function.

Changing existing code is laborious and time consuming, plus there are the issues it causes with glt blame and patches across branches etc but I that doesn't change the fact this style would be safer, more concise and preferable overall.

i do not disagree. but we are not going to go on and change every part of the code to do this.
we are just going to cause more disruptions to ongoing prs, feature branches that is not going to give that much gain.

so i will approve the change for this file, because i know there is not much work going on in the object api area. @jxiong but please refrain from pushing such a change more broadly before discussing this more in developer channels and/or gatekeeping channels.

@mchaarawi are you in the community slack channel? My understanding is this PR was created to support a discussion there.

I am. this is not a community question though. this is developer specific topic.

jxiong · 2024-11-12T22:44:35Z

i do not get the purpose of doing these changes on just this file. this screws up git blame. we probably have many other files that do not have clang format and i will request we don't do such changes for now unless we make a discussion to address this everywhere.

I agree with you that we should not make PRs for this change only. This is just to make an example of such change. The PR description gives a clearer explanation on this. The goal is sort of to announce that this is a preferred way of defining loop variables.

mchaarawi · 2024-11-12T22:49:26Z

i do not get the purpose of doing these changes on just this file. this screws up git blame. we probably have many other files that do not have clang format and i will request we don't do such changes for now unless we make a discussion to address this everywhere.

I agree with you that we should not make PRs for this change only. This is just to make an example of such change. The PR description gives a clearer explanation on this. The goal is sort of to announce that this is a preferred way of defining loop variables.

i think step 1 would be to revert all the clang format changes in the file. you should not format code that you did not touch. I am not ready to loose git blame information on all files :-)
We use git blame to assign issues in some situations.

for the loop variable declaration, this is not something we discussed or adopted, so we cannot force it on everyone without a discussion in gatekeeping at least.

jxiong · 2024-11-12T22:58:52Z

i do not get the purpose of doing these changes on just this file. this screws up git blame. we probably have many other files that do not have clang format and i will request we don't do such changes for now unless we make a discussion to address this everywhere.

I agree with you that we should not make PRs for this change only. This is just to make an example of such change. The PR description gives a clearer explanation on this. The goal is sort of to announce that this is a preferred way of defining loop variables.

i think step 1 would be to revert all the clang format changes in the file. you should not format code that you did not touch. I am not ready to loose git blame information on all files :-) We use git blame to assign issues in some situations.

for the loop variable declaration, this is not something we discussed or adopted, so we cannot force it on everyone without a discussion in gatekeeping at least.

That's absolutely reasonable. I don't have any background on this topic but it seems like we have a clang-format in place but it's not actually in use. (I have dev config that clang-format will be invoked automatically on file save).

I will revert the clang-format change and resubmit.

Change-Id: I8c6b78f95b3737f604ac87b4d16636202074c47b

daosbuild1 · 2024-11-13T00:29:30Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15428/6/testReport/

jxiong · 2024-11-15T21:08:14Z

@jolivier23 @ashleypittman can anyone of you please restart the CI for me? Thanks

ashleypittman · 2024-11-18T08:18:49Z

@jolivier23 @ashleypittman can anyone of you please restart the CI for me? Thanks

You will need to merge with master to pickup the code fix, master was failing for a few days at the start of last week. Master was fixed on Tuesday and Existing approvals should be maintained.

Change-Id: I476cc125e3370056c660cb2d01b935e4b15abd1b

daosbuild1 · 2024-11-18T18:41:56Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15428/7/testReport/

Change-Id: Idd9b7044693bd551ec99b7e1c66a505e11dd49b3

jxiong · 2024-11-22T21:23:51Z

NLT is broken. @daos-stack/daos-gatekeeper can we just land it as-is?

mchaarawi · 2024-11-22T21:28:12Z

Please refrain from using DAOS-623 in the future. thanks :-)

format: adopt C99 standard

013c1cf

jxiong requested review from a team as code owners October 30, 2024 01:08

jxiong marked this pull request as draft October 30, 2024 01:08

wangshilong previously approved these changes Oct 30, 2024

View reviewed changes

jolivier23 previously approved these changes Oct 30, 2024

View reviewed changes

jxiong changed the title ~~[NO TICKET] format: allow variable declaration in loop~~ DAOS-623 format: allow variable declaration in loop Oct 30, 2024

jxiong self-assigned this Oct 30, 2024

jxiong marked this pull request as ready for review October 30, 2024 20:16

jxiong changed the title ~~DAOS-623 format: allow variable declaration in loop~~ DAOS-623 client: allow variable declaration in loop Oct 30, 2024

format the code with d_logging_check.py

5c8a986

jxiong dismissed stale reviews from jolivier23 and wangshilong via 5c8a986 November 4, 2024 17:56

ashleypittman previously approved these changes Nov 5, 2024

View reviewed changes

jxiong requested review from wangshilong and jolivier23 November 5, 2024 18:08

jolivier23 previously approved these changes Nov 5, 2024

View reviewed changes

jxiong added 2 commits November 5, 2024 20:07

Merge remote-tracking branch 'intel/master' into jxiong/variable_in_loop

046c899

Merge remote-tracking branch 'ups/master' into jxiong/variable_in_loop

d72d8d5

jxiong requested a review from a team November 8, 2024 17:16

jxiong requested a review from mchaarawi November 12, 2024 22:33

mchaarawi requested changes Nov 12, 2024

View reviewed changes

revert clang-format change

4bc1807

Change-Id: I8c6b78f95b3737f604ac87b4d16636202074c47b

jxiong dismissed stale reviews from ashleypittman and jolivier23 via 4bc1807 November 12, 2024 23:47

jxiong requested review from mchaarawi, jolivier23 and ashleypittman November 14, 2024 17:11

mchaarawi approved these changes Nov 14, 2024

View reviewed changes

ashleypittman approved these changes Nov 14, 2024

View reviewed changes

Merge remote-tracking branch 'ups/master' into jxiong/variable_in_loop

b414669

Change-Id: I476cc125e3370056c660cb2d01b935e4b15abd1b

Merge remote-tracking branch 'ups/master' into jxiong/variable_in_loop

32c0e31

Change-Id: Idd9b7044693bd551ec99b7e1c66a505e11dd49b3

mchaarawi merged commit dab82b8 into master Nov 22, 2024
51 of 55 checks passed

mchaarawi deleted the jxiong/variable_in_loop branch November 22, 2024 21:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-623 client: allow variable declaration in loop #15428

DAOS-623 client: allow variable declaration in loop #15428

jxiong commented Oct 30, 2024 •

edited

Loading

github-actions bot commented Oct 30, 2024 •

edited

Loading

wangshilong left a comment •

edited

Loading

jolivier23 commented Oct 30, 2024

jolivier23 left a comment

jxiong commented Oct 30, 2024

jxiong commented Nov 7, 2024

ashleypittman commented Nov 7, 2024

daltonbohning commented Nov 12, 2024

mchaarawi left a comment

mchaarawi Nov 12, 2024

ashleypittman Nov 14, 2024

mchaarawi Nov 14, 2024

ashleypittman Nov 14, 2024

mchaarawi Nov 14, 2024

jxiong commented Nov 12, 2024

mchaarawi commented Nov 12, 2024 •

edited

Loading

jxiong commented Nov 12, 2024

daosbuild1 commented Nov 13, 2024

jxiong commented Nov 15, 2024

ashleypittman commented Nov 18, 2024

daosbuild1 commented Nov 18, 2024

jxiong commented Nov 22, 2024

mchaarawi commented Nov 22, 2024

DAOS-623 client: allow variable declaration in loop #15428

DAOS-623 client: allow variable declaration in loop #15428

Conversation

jxiong commented Oct 30, 2024 • edited Loading

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Oct 30, 2024 • edited Loading

wangshilong left a comment • edited Loading

Choose a reason for hiding this comment

jolivier23 commented Oct 30, 2024

jolivier23 left a comment

Choose a reason for hiding this comment

jxiong commented Oct 30, 2024

jxiong commented Nov 7, 2024

ashleypittman commented Nov 7, 2024

daltonbohning commented Nov 12, 2024

mchaarawi left a comment

Choose a reason for hiding this comment

mchaarawi Nov 12, 2024

Choose a reason for hiding this comment

ashleypittman Nov 14, 2024

Choose a reason for hiding this comment

mchaarawi Nov 14, 2024

Choose a reason for hiding this comment

ashleypittman Nov 14, 2024

Choose a reason for hiding this comment

mchaarawi Nov 14, 2024

Choose a reason for hiding this comment

jxiong commented Nov 12, 2024

mchaarawi commented Nov 12, 2024 • edited Loading

jxiong commented Nov 12, 2024

daosbuild1 commented Nov 13, 2024

jxiong commented Nov 15, 2024

ashleypittman commented Nov 18, 2024

daosbuild1 commented Nov 18, 2024

jxiong commented Nov 22, 2024

mchaarawi commented Nov 22, 2024

jxiong commented Oct 30, 2024 •

edited

Loading

github-actions bot commented Oct 30, 2024 •

edited

Loading

wangshilong left a comment •

edited

Loading

mchaarawi commented Nov 12, 2024 •

edited

Loading