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

Clean utils args #596

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

Conversation

clementFoyer
Copy link
Collaborator

Mostly code refactoring.

Unify the definition of options to increase code readability. Reorder the option checks to enforce topology options to be parsed first. Unmix using tabs and spaces (excepted for cases when the line is cut due to the function's arguments). Enforce using NULL character instead of simply 0.

sizevalue *= 1024ULL*1024ULL*1024ULL*1024ULL;
else if (!strcasecmp(end, "TiB"))
sizevalue <<= 40;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems that the treatment is the same whether the extension is [kMGT]B or [kMGT]iB. Isn't shifting left 10 times and multiplying by 1024 the same ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like commit 4d9a984 failed to fix [kMGT]B contrary to what the commit message said :)

@bgoglin
Copy link
Contributor

bgoglin commented Jun 13, 2023

First 2 commits are OK. 3rd one will require 3 months of careful review and mixes several things. I don't like the 4th commit because it makes git blame a bit harder. I usually reindent only when the modify the code nearby.

@bgoglin
Copy link
Contributor

bgoglin commented Jun 13, 2023

I pushed the first two commits and fixed the hwloc-annotate bug with units.

@clementFoyer
Copy link
Collaborator Author

I can split it by files and/or by changes (although the changes that are not just the reordering of the options are usually close to it).

No problem for the fourth commit. I think you may have told me in the past, but I had forgotten it. Anyhow, I actually put in a separated commit because I knew it was more likely to be refused.

@bgoglin
Copy link
Contributor

bgoglin commented Jun 14, 2023

I pushed the first commit, I'll try to review the last one later today.

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.

2 participants