Skip to content

Eliminate clang-tidy dependency #1591

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Eliminate clang-tidy dependency #1591

wants to merge 1 commit into from

Conversation

dime10
Copy link
Contributor

@dime10 dime10 commented Mar 28, 2025

This is just a proposal, let me know if there are any objections :)

With lightning devices migrated out of Catalyst, the only remaining code affected by the clang-tidy option is the OpenQASM device code. The option is also disabled by default and not run in any CI scripts, so I propose to remove it entirely for simplicity.

With lightning devices migrated out of Catalyst, only the OpenQASM
device code was using it, plus it was disabled by default and not run
in any CI scripts.
@dime10 dime10 added runtime Pull requests that update the runtime housecleaning labels Mar 28, 2025
@dime10 dime10 requested a review from a team March 28, 2025 20:15
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md on your branch with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@mlxd
Copy link
Member

mlxd commented Mar 28, 2025

I assumed the audit PR was enabling this for WAE. I'd rather we enable CT for the runtime, and wherever else if possible.
I can undelete my PR to add this into the pipeline.

Update: Reopened #1098

@dime10
Copy link
Contributor Author

dime10 commented Mar 28, 2025

I assumed the audit PR was enabling this for WAE.

They are orthogonal features no? My understanding is clang-tidy is more like a linter?

I'd rather we enable CT for the runtime, and wherever else if possible.

Sounds good, if you'd rather build out the clang-tidy support that's fair (in the current state it was just useless, hence the PR).
Personally I would be cautious of "over-linting" (from our experience with pylint and codefactor leading to a lot of wasted time), but if we're selective about the rules than I would be in favour :)

@mlxd
Copy link
Member

mlxd commented Mar 31, 2025

I assumed the audit PR was enabling this for WAE.

They are orthogonal features no? My understanding is clang-tidy is more like a linter?

I'd rather we enable CT for the runtime, and wherever else if possible.

Sounds good, if you'd rather build out the clang-tidy support that's fair (in the current state it was just useless, hence the PR). Personally I would be cautious of "over-linting" (from our experience with pylint and codefactor leading to a lot of wasted time), but if we're selective about the rules than I would be in favour :)

Sure thing. All the rules are in https://github.com/PennyLaneAI/guidance-docs/blob/master/config_files/.clang-tidy already with the reasoning in the associated dev docs :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housecleaning runtime Pull requests that update the runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants