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

Introduce clang-tidy #878

Open
wants to merge 16 commits into
base: dev
Choose a base branch
from
Open

Introduce clang-tidy #878

wants to merge 16 commits into from

Conversation

robertwoj-microsoft
Copy link
Contributor

@robertwoj-microsoft robertwoj-microsoft commented Feb 18, 2025

Description

Introduce clang-tidy to the project. Currently it only works on the src/modules/compliance directory.
This PR depends on #873, DO NOT MERGE YET

Checklist

  • I have read the contribution guidelines.
  • All unit tests are passing.
  • I have merged the latest dev branch prior to this PR submission.
  • I ran pre-commit on my changes prior to this PR submission.
  • I submitted this PR against the dev branch.

@robertwoj-microsoft robertwoj-microsoft requested a review from a team as a code owner February 18, 2025 10:14
Copy link

github-actions bot commented Feb 18, 2025

Test Results

 38 files   - 2   38 suites   - 2   34m 37s ⏱️ - 1m 54s
  4 tests ±0    4 ✅ ±0   0 💤 ±0  0 ❌ ±0 
152 runs   - 8  134 ✅  - 6  18 💤  - 2  0 ❌ ±0 

Results for commit 01aacc9. ± Comparison against base commit 62f0d9e.

♻️ This comment has been updated with latest results.

@robertwoj-microsoft robertwoj-microsoft changed the title clang-tidy: CamelCase clang-tidy: Work In Progress Feb 18, 2025
@@ -61,7 +61,7 @@ Result<std::string> Base64Decode(const std::string& input)

for (size_t i = 0; i < input.size(); i += 4)
{
unsigned char enc[4];
char enc[4];
Copy link
Contributor

Choose a reason for hiding this comment

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

char enc[4];

Add explicit initialization to all zeroes with '= {0}'? Here and elsewhere

@@ -36,10 +36,11 @@ class Engine

AuditResult(const AuditResult&) = delete;
AuditResult(AuditResult&& other) noexcept
: result(other.result)
, payload(other.payload)
Copy link
Contributor

@MariusNi MariusNi Feb 18, 2025

Choose a reason for hiding this comment

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

,

This is unusual, not used anywhere else in this project. Can we move the commas (,) to the end of previous line instead? Keep the column (:) as-is.

Like this:

AuditResult(AuditResult&& other) noexcept
            : result(other.result),
            payload(other.payload).
            payloadSize(other.payloadSize)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is unusual, not used anywhere else in this project.

This is C++-specific - member initialization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated here: 4dd7321 which belongs to PR #873

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just informational comment: putting commas in front of the initializer makes much easier to add new fields and reorder them in case of class definition changes.

MariusNi
MariusNi previously approved these changes Feb 18, 2025
Copy link
Contributor

@MariusNi MariusNi left a comment

Choose a reason for hiding this comment

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

:shipit:

@robertwoj-microsoft robertwoj-microsoft changed the title clang-tidy: Work In Progress Introduce clang-tidy Feb 19, 2025
AhmedBM
AhmedBM previously approved these changes Feb 20, 2025
@robertwoj-microsoft robertwoj-microsoft force-pushed the compliance/code_formatting branch 2 times, most recently from 30874ce to 033c331 Compare February 21, 2025 17:51
Base automatically changed from compliance/code_formatting to dev February 21, 2025 18:07
@MariusNi MariusNi dismissed stale reviews from AhmedBM and themself February 21, 2025 18:07

The base branch was changed.

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