Skip to content

[CI] Add clang-tidy check with clang-analyzer and performance checks #490

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

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented May 20, 2025

Fixes #200

Code is fixed mostly due to the following checks, especially for the performance-unnecessary-value-param because the existing code passes std::shared_ptr and std::function by value in many places, which harms the performance. The virtual call in the destructor like ProducerImpl::shutdown is also fixed.

Reference: https://clang.llvm.org/extra/clang-tidy/checks/list.html

Safety

  • clang-analyzer-optin.cplusplus.VirtualCall
  • clang-analyzer-cplusplus.Move

Performance

  • clang-analyzer-optin.performance.Padding
  • performance-enum-size
  • performance-noexcept-move-constructor
  • performance-unnecessary-value-param
  • performance-faster-string-find
  • performance-avoid-endl
  • performance-move-const-arg
  • performance-unnecessary-copy-initialization

Code Quality

  • clang-analyzer-deadcode.DeadStore

@BewareMyPower BewareMyPower marked this pull request as draft May 20, 2025 12:31
@BewareMyPower BewareMyPower self-assigned this May 20, 2025
@BewareMyPower BewareMyPower added this to the 3.8.0 milestone May 20, 2025
@BewareMyPower BewareMyPower added style enhancement New feature or request and removed build labels May 20, 2025
@BewareMyPower BewareMyPower changed the title [CI] Add clang-tidy check [CI] Add clang-tidy check with clang-analyzer and performance checks May 21, 2025
@BewareMyPower BewareMyPower force-pushed the bewaremypower/clang-tidy branch from f00b3c2 to 3a793de Compare May 21, 2025 15:08
@BewareMyPower BewareMyPower marked this pull request as ready for review May 22, 2025 04:57
@RobertIndie RobertIndie requested a review from Copilot May 23, 2025 07:25
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request adds enhanced clang-tidy checks—including clang-analyzer and performance checks—to improve code quality and runtime efficiency by addressing unnecessary value parameters and virtual calls.

  • Updates function signatures to pass callbacks, strings, and shared pointers as const references to avoid extra copies.
  • Explicitly sets enum underlying types to uint8_t for consistency and potential performance improvements.
  • Adds CI workflow steps for linting and clang-tidy analysis.

Reviewed Changes

Copilot reviewed 123 out of 123 changed files in this pull request and generated no comments.

Show a summary per file
File Description
include/pulsar/ProducerConfiguration.h Added include and updated enum declarations and parameter types.
include/pulsar/Producer.h Revised asynchronous callback parameters and shared pointer constructor.
include/pulsar/Logger.h Updated enum declaration with underlying type specifier.
include/pulsar/KeySharedPolicy.h Updated enum declaration with underlying type specifier.
include/pulsar/InitialPosition.h Updated enum declaration with underlying type specifier.
include/pulsar/FileLoggerFactory.h Added missing include.
include/pulsar/EncryptionKeyInfo.h Changed setKey signature and adjusted constructor parameter type.
include/pulsar/CryptoKeyReader.h Updated file parameter signature to const reference.
include/pulsar/ConsumerType.h Updated enum declaration with underlying type specifier.
include/pulsar/ConsumerConfiguration.h Updated setter signature to use a const reference.
include/pulsar/Consumer.h Revised several asynchronous callback parameters to use const references.
include/pulsar/ConsoleLoggerFactory.h Added missing include.
include/pulsar/CompressionType.h Updated enum declaration with underlying type specifier.
include/pulsar/ClientConfiguration.h Updated enum declaration with underlying type specifier and includes.
include/pulsar/Client.h Revised asynchronous API signatures to pass parameters by const reference.
include/pulsar/Authentication.h Replaced an enum with a static constexpr for undefined_expiration.
build-support/run_clang_tidy.sh Added a new script to run clang-tidy across the codebase.
CMakeLists.txt Adjusted compiler options per updated guidelines.
.github/workflows/ci-pr-validation.yaml Introduced new lint jobs and updated build configurations.
.clang-tidy Added configuration for clang-tidy checks.
Comments suppressed due to low confidence (3)

include/pulsar/Producer.h:164

  • Changing the Producer constructor to take a shared pointer by const reference instead of by value may affect reference counting and lifetime management; please ensure that a copy is made internally if needed to maintain expected ownership semantics.
explicit Producer(const ProducerImplBasePtr&);

include/pulsar/Consumer.h:86

  • Converting asynchronous callback parameters to const references improves performance, but please confirm that their lifetimes are properly managed (for example, by copying them if they are stored for later invocation) to avoid potential dangling references.
void unsubscribeAsync(const ResultCallback& callback);

include/pulsar/Client.h:104

  • Ensure that passing the callback as a const reference for asynchronous operations does not introduce lifetime issues; if the callback is stored beyond the function call, an internal copy should be made.
void createProducerAsync(const std::string& topic, const CreateProducerCallback& callback);

auto numProducers = producers.size();
auto numConsumers = consumers.size();
// Call the destructors out of the lock to avoid deadlocks
producers.clear();
Copy link
Member

Choose a reason for hiding this comment

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

The producer in the map producers is a weak ptr. Calling the clear here doesn't guarantee the desturctor will be called. So the shutdown may not be called here. However, I think we should make sure all the producers/consumers should be shutdown after the client shutdown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good find! It makes sense. Let me think of a better solution.

python3 $SCRIPT -p build -j$(nproc) $(cat files.txt)
else
python3 $SCRIPT -p build -j8 $(cat files.txt)
fi
Copy link
Member

Choose a reason for hiding this comment

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

Should we delete the files.txt here?

@BewareMyPower BewareMyPower marked this pull request as draft May 23, 2025 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request style
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improve] Apply clang-tidy check in CI
2 participants