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

Solved some clang-tidy complaints #224

Merged
merged 11 commits into from
Jul 18, 2022
Merged

Solved some clang-tidy complaints #224

merged 11 commits into from
Jul 18, 2022

Conversation

EduardGomezEscandell
Copy link
Collaborator

@EduardGomezEscandell EduardGomezEscandell commented Jul 15, 2022

Only made changes in non-microsoft files. Applied most clang-tidy feedback from the last run in main (https://github.com/ubuntu/WSL/runs/7303623016?check_suite_focus=true).

Solved

  • Most short-variable-name complaints
  • Adding some consts
  • NULL -> nullptr
  • Removed some unused variable names in lambda arguments. Example: [&](auto&&... s) { return E_UNEXPECTED; }, becomes [&](auto&&...) { return E_UNEXPECTED; },.
  • Order of constructor list

Not solved

  • Reasonable or standard short variable names (e.g: ok or hr)
  • Some variables that I wasn't sure what to rename them to.
  • Implicit casts from BOOL (Microsoft's typedef'd int) to bool.

@EduardGomezEscandell EduardGomezEscandell added the low Low importance bug label Jul 15, 2022
@EduardGomezEscandell EduardGomezEscandell self-assigned this Jul 15, 2022
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Please check out carefully the isses found by clang-tidy in the new code ⚠️
⚠️ clang-format found formatting issues in the code submitted.:warning:
Make sure to run clang-format and update this pull request.
(1/1)

DistroLauncher/local_named_pipe.h Outdated Show resolved Hide resolved
DistroLauncher/local_named_pipe.h Outdated Show resolved Hide resolved
DistroLauncher/local_named_pipe.h Outdated Show resolved Hide resolved
DistroLauncher/ApplicationStrategy.cpp Show resolved Hide resolved
DistroLauncher/ProcessRunner.cpp Show resolved Hide resolved
DistroLauncher/ProcessRunner.cpp Outdated Show resolved Hide resolved
DistroLauncher/ProcessRunner.cpp Outdated Show resolved Hide resolved
DistroLauncher/ProcessRunner.cpp Outdated Show resolved Hide resolved
DistroLauncher/ProcessRunner.cpp Outdated Show resolved Hide resolved
EduardGomezEscandell and others added 3 commits July 15, 2022 10:29
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@CarlosNihelton
Copy link
Collaborator

CarlosNihelton commented Jul 15, 2022

Not solved

  • Reasonable or standard short variable names (e.g: ok or hr)

Would you say we should tweak some parameters of the [readability-identifier-length check] ?(https://releases.llvm.org/14.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/readability-identifier-length.html)

  • Implicit casts from BOOL (Microsoft's typedef'd int) to bool.

For comparisons (like if(!bSuccess)) we can compare to TRUE or FALSE instead of implicitly converting to bool. For situations where we pass bool as BOOL we can static_cast<BOOL>.

@EduardGomezEscandell
Copy link
Collaborator Author

EduardGomezEscandell commented Jul 18, 2022

For comparisons (like if(!bSuccess)) we can compare to TRUE or FALSE instead of implicitly converting to bool. For situations where we pass bool as BOOL we can static_cast.

Agreed and changed.

Would you say we should tweak some parameters of the [readability-identifier-length check] ?

I'd say we should allow hr for variables of type HRESULT as it is the de facto standard. There is only a couple of instances with variables named ok, here I think we can change them to status or success depending on their semantics.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Please check out carefully the isses found by clang-tidy in the new code ⚠️
⚠️ clang-format found formatting issues in the code submitted.:warning:
Make sure to run clang-format and update this pull request.
(1/1)

DistroLauncher/state_machine.h Outdated Show resolved Hide resolved
DistroLauncher/ProcessRunner.cpp Show resolved Hide resolved
DistroLauncher/ProcessRunner.cpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Please check out carefully the isses found by clang-tidy in the new code ⚠️
⚠️ clang-format found formatting issues in the code submitted.:warning:
Make sure to run clang-format and update this pull request.
(1/1)

DistroLauncher/state_machine.h Show resolved Hide resolved
DistroLauncher/state_machine.h Show resolved Hide resolved
Copy link
Collaborator

@CarlosNihelton CarlosNihelton left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! Really appreciated. Would you mind squashing this one as well?

@EduardGomezEscandell EduardGomezEscandell merged commit d63b1a2 into main Jul 18, 2022
@EduardGomezEscandell EduardGomezEscandell deleted the clang-tidying branch July 18, 2022 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
low Low importance bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants