[clang-tidy] Check for `-ignore-insert-conflict` support before using it in `apply_fixes` in `run-clang-tidy.py` #127066

loumalouomega · 2025-02-13T14:34:54Z

Check for -ignore-insert-conflict support before using it in apply_fixes in run-clang-tidy.py

Details:

Added a helper function supports_flag() to check if clang-apply-replacements supports -ignore-insert-conflict by running --help.
Prevents potential errors when using older versions that don't support this flag.
Ensures compatibility and robustness when invoking clang-apply-replacements.

…ng-tidy.py`

github-actions · 2025-02-13T14:35:15Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-02-13T14:35:46Z

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Vicente Mataix Ferrándiz (loumalouomega)

Changes

Remove legacy argument -ignore-insert-conflict flag from run-clang-tidy.py

Summary:

This PR removes the -ignore-insert-conflict flag from the invocation of the clang-apply-replacements tool in the run-clang-tidy.py script. The flag was previously added to the list of arguments passed to clang-apply-replacements, but it has been deemed unnecessary and cause of errors, and therefore, it was deleted.

Full diff: https://github.com/llvm/llvm-project/pull/127066.diff

1 Files Affected:

(modified) clang-tools-extra/clang-tidy/tool/run-clang-tidy.py (-1)

diff --git a/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py b/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
index f1b934f7139e9..acee87c860aa0 100755
--- a/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ b/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -200,7 +200,6 @@ def apply_fixes(
 ) -> None:
     """Calls clang-apply-fixes on a given directory."""
     invocation = [clang_apply_replacements_binary]
-    invocation.append("-ignore-insert-conflict")
     if args.format:
         invocation.append("-format")
     if args.style:

HerrCai0907 · 2025-02-13T14:58:58Z

Remove legacy argument -ignore-insert-conflict flag from run-clang-tidy.py

Could you explain more about legacy argument -ignore-insert-conflict. It still in clang-apply-replacements tool.

USAGE: clang-apply-replacements [options] <Search Root Directory>

OPTIONS:

Formatting Options:

  --format                   - Enable formatting of code changed by applying replacements.
                               Use -style to choose formatting style.
  --style=<string>           - Set coding style. <string> can be:
                               1. A preset: LLVM, GNU, Google, Chromium, Microsoft,
                                  Mozilla, WebKit.
                               2. 'file' to load style configuration from a
                                  .clang-format file in one of the parent directories
                                  of the source file (for stdin, see --assume-filename).
                                  If no .clang-format file is found, falls back to
                                  --fallback-style.
                                  --style=file is the default.
                               3. 'file:<format_file_path>' to explicitly specify
                                  the configuration file.
                               4. "{key: value, ...}" to set specific parameters, e.g.:
                                  --style="{BasedOnStyle: llvm, IndentWidth: 8}"
  --style-config=<string>    - Path to a directory containing a .clang-format file
                               describing a formatting style to use for formatting
                               code when -style=file.

Generic Options:

  --help                     - Display available options (--help-hidden for more)
  --help-list                - Display list of available options (--help-list-hidden for more)
  --version                  - Display the version of this program

Replacement Options:

  --ignore-insert-conflict   - Ignore insert conflict and keep running to fix.
  --remove-change-desc-files - Remove the change description files regardless of successful
                               merging/replacing.

loumalouomega · 2025-02-13T15:03:00Z

Remove legacy argument -ignore-insert-conflict flag from run-clang-tidy.py

Could you explain more about legacy argument -ignore-insert-conflict. It still in clang-apply-replacements tool.

USAGE: clang-apply-replacements [options] <Search Root Directory>

OPTIONS:

Formatting Options:

  --format                   - Enable formatting of code changed by applying replacements.
                               Use -style to choose formatting style.
  --style=<string>           - Set coding style. <string> can be:
                               1. A preset: LLVM, GNU, Google, Chromium, Microsoft,
                                  Mozilla, WebKit.
                               2. 'file' to load style configuration from a
                                  .clang-format file in one of the parent directories
                                  of the source file (for stdin, see --assume-filename).
                                  If no .clang-format file is found, falls back to
                                  --fallback-style.
                                  --style=file is the default.
                               3. 'file:<format_file_path>' to explicitly specify
                                  the configuration file.
                               4. "{key: value, ...}" to set specific parameters, e.g.:
                                  --style="{BasedOnStyle: llvm, IndentWidth: 8}"
  --style-config=<string>    - Path to a directory containing a .clang-format file
                               describing a formatting style to use for formatting
                               code when -style=file.

Generic Options:

  --help                     - Display available options (--help-hidden for more)
  --help-list                - Display list of available options (--help-list-hidden for more)
  --version                  - Display the version of this program

Replacement Options:

  --ignore-insert-conflict   - Ignore insert conflict and keep running to fix.
  --remove-change-desc-files - Remove the change description files regardless of successful
                               merging/replacing.

In version 14.0 looks like is not anymore:

clang-apply-replacements --version 
clang-apply-replacements version 14.0.0

clang-apply-replacements --help
USAGE: clang-apply-replacements [options] <Search Root Directory>

OPTIONS:

Formatting Options:

  --format                   - Enable formatting of code changed by applying replacements.
                               Use -style to choose formatting style.
  --style=<string>           - Coding style, currently supports:
                                 LLVM, GNU, Google, Chromium, Microsoft, Mozilla, WebKit.
                               Use -style=file to load style configuration from
                               .clang-format file located in one of the parent
                               directories of the source file (or current
                               directory for stdin).
                               Use -style=file:<format_file_path> to explicitly specifythe configuration file.
                               Use -style="{key: value, ...}" to set specific
                               parameters, e.g.:
                                 -style="{BasedOnStyle: llvm, IndentWidth: 8}"
  --style-config=<string>    - Path to a directory containing a .clang-format file
                               describing a formatting style to use for formatting
                               code when -style=file.

Generic Options:

  --help                     - Display available options (--help-hidden for more)
  --help-list                - Display list of available options (--help-list-hidden for more)
  --version                  - Display the version of this program

Replacement Options:

  --remove-change-desc-files - Remove the change description files regardless of successful
                               merging/replacing.

PiotrZSL · 2025-02-13T16:57:25Z

In version 14.0 looks like is not anymore:

Because, it were added in Clang 15.
Even now on main branch it's still utilized.

PiotrZSL

Best would be to add that option to script and let user decide whatever use it or not.

loumalouomega · 2025-02-13T17:01:04Z

Best would be to add that option to script and let user decide whatever use it or not.

Do you want me to do it?

loumalouomega · 2025-02-13T17:02:08Z

Ok, so is not legacy, but the contrary. My version of Clang is old. I can refactor if you want.

loumalouomega · 2025-02-14T10:55:46Z

@PiotrZSL FYI

clang-tools-extra/clang-tidy/tool/run-clang-tidy.py

carlosgalvezp

LGTM except for some minor nits. Also, the commit message is misleading, it should be something like:

[clang-tidy] Add support for `-ignore-insert-conflict` in `run-clang-tidy.py`

loumalouomega · 2025-02-14T11:20:52Z

LGTM except for some minor nits. Also, the commit message is misleading, it should be something like:
[clang-tidy] Add support for `-ignore-insert-conflict` in `run-clang-tidy.py`

Okay, I don'w know the style. BTW,I applied tour comments

clang-tools-extra/clang-tidy/tool/run-clang-tidy.py

HerrCai0907

LGTM. Please update release note also.
After that, feel free to ping me to merge it if you do not have permission.

github-actions · 2025-02-14T23:33:59Z

⚠️ Python code formatter, darker found issues in your code. ⚠️

You can test this locally with the following command:

darker --check --diff -r 4664a4c66b816af53f596935c3aaa2eca143ae9c...80b0b797a4abb48d37455651c1ac070df00f6344 clang-tools-extra/clang-tidy/tool/run-clang-tidy.py

View the diff from darker here.

--- run-clang-tidy.py	2025-02-14 11:25:07.000000 +0000
+++ run-clang-tidy.py	2025-02-16 01:14:10.095163 +0000
@@ -198,11 +198,11 @@
 def apply_fixes(
     args: argparse.Namespace, clang_apply_replacements_binary: str, tmpdir: str
 ) -> None:
     """Calls clang-apply-replacements on a given directory."""
     invocation = [clang_apply_replacements_binary]
-    
+
     if args.ignore_insert_conflict:
         invocation.append("-ignore-insert-conflict")
     if args.format:
         invocation.append("-format")
     if args.style:

loumalouomega · 2025-02-15T16:32:08Z

LGTM. Please update release note also. After that, feel free to ping me to merge it if you do not have permission.

Can you help me?, I have no experience in this repository

nicovank · 2025-02-15T17:15:26Z

clang-tools-extra/clang-tidy/tool/run-clang-tidy.py

+    parser.add_argument(
+        "-ignore-insert-conflict",
+        action="store_true",
+        default=True,


Shouldn't this be false by default and toggled to true when the option is present?

https://github.com/llvm/llvm-project/pull/127066/files/80b0b797a4abb48d37455651c1ac070df00f6344#r1955988200

I was asked to preserve current behaviour

HerrCai0907 · 2025-02-16T01:11:01Z

Can you help me?, I have no experience in this repository

release-note is placed clang-tools-extra/docs/ReleaseNotes.rst. You can follow the this format to add
new item. https://releases.llvm.org/19.1.0/tools/clang/tools/extra/docs/ReleaseNotes.html#improvements-to-clang-tidy

clang-tools-extra/clang-tidy/tool/run-clang-tidy.py

… it in `apply_fixes` in `run_clang_tidy.py`

whisperity · 2025-02-17T13:59:20Z

I am a bit confused here. If you're using the system Clang toolchain assembled by your company (outside of your control), that toolchain should have delivered a run-clang-tidy.py that is appropriate for that version.

If you are using run-clang-tidy.py from the Git working directory obtained from upstream sources (i.e., run-clang-tidy.py is newer and has this flag), then all the other tools (including clang-tidy and clang-apply-replacements) should be the appropriate newer version.

Otherwise you might be mixing versions, standard library implementations, intrinsic headers, etc., and there will be plenty of subtle bugs lurking behind every corner!

So in general, what was the original point you're trying to achieve? To me, the solution to the lack of -ignore-insert-conflict in clang-apply-replacements would be to build a newer clang-apply-replacements and have that in the PATH earlier than the system version. So your local clang-tidy binary, the clang-apply-replacements, and the run-clang-tidy.py are all appropriately from the same version.

loumalouomega · 2025-02-17T14:33:09Z

I am a bit confused here. If you're using the system Clang toolchain assembled by your company (outside of your control), that toolchain should have delivered a run-clang-tidy.py that is appropriate for that version.

If you are using run-clang-tidy.py from the Git working directory obtained from upstream sources (i.e., run-clang-tidy.py is newer and has this flag), then all the other tools (including clang-tidy and clang-apply-replacements) should be the appropriate newer version.

Otherwise you might be mixing versions, standard library implementations, intrinsic headers, etc., and there will be plenty of subtle bugs lurking behind every corner!

So in general, what was the original point you're trying to achieve? To me, the solution to the lack of -ignore-insert-conflict in clang-apply-replacements would be to build a newer clang-apply-replacements and have that in the PATH earlier than the system version. So your local clang-tidy binary, the clang-apply-replacements, and the run-clang-tidy.py are all appropriately from the same version.

I have a certain version of Ubuntu with a certain version of Clang, not custom compiled.

carlosgalvezp · 2025-02-17T14:59:39Z

should have delivered a run-clang-tidy.py that is appropriate for that version.

It should, but does it? A typical installation comes from installing the Ubuntu packages, do they include run-clang-tidy.py at all?

If not, then I don't think run-clang-tidy.py should have the expectations of backwards compatibility and so on that we have for the more "public" deliveries. In that case, I agree with @whisperity that you should use a matching version.

You are not in control of the Clang/clang-tidy packages, but what about run-clang-tidy.py, @loumalouomega ? Can you downgrade that instead, to a version that matches the Clang/clang-tidy installation you have? I mean you should be able to clone this repo and checkout a compatible version, right? Since you already can clone the repo and put up a PR.

loumalouomega · 2025-02-17T15:10:06Z

should have delivered a run-clang-tidy.py that is appropriate for that version.

It should, but does it? A typical installation comes from installing the Ubuntu packages, do they include run-clang-tidy.py at all?

If not, then I don't think run-clang-tidy.py should have the expectations of backwards compatibility and so on that we have for the more "public" deliveries. In that case, I agree with @whisperity that you should use a matching version.

You are not in control of the Clang/clang-tidy packages, but what about run-clang-tidy.py, @loumalouomega ? Can you downgrade that instead, to a version that matches the Clang/clang-tidy installation you have? I mean you should be able to clone this repo and checkout a compatible version, right? Since you already can clone the repo and put up a PR.

The script is not included in the installation, and I cannot install them, thats why I downloaded manually the script and detected the problem. I can close this, if it is too problematic. I just wanted some people to not suffer this problematic in the future.

whisperity · 2025-02-19T12:55:18Z

I have a certain version of Ubuntu with a certain version of Clang, not custom compiled.

@loumalouomega Can you please confirm if the Clang comes from the official Debian or Ubuntu APT repository?

If that is the case, I think we can, going forward, contact the maintainers of that pipeline (if the patch can't be done locally within the LLVM repo) to include this script as long as this script can be considered "user-facing" and not a developer-only tool.

loumalouomega · 2025-02-19T13:20:51Z

I have a certain version of Ubuntu with a certain version of Clang, not custom compiled.

@loumalouomega Can you please confirm if the Clang comes from the official Debian or Ubuntu APT repository?

If that is the case, I think we can, going forward, contact the maintainers of that pipeline (if the patch can't be done locally within the LLVM repo) to include this script as long as this script can be considered "user-facing" and not a developer-only tool.

lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 22.04.5 LTS
Release:        22.04
Codename:       jammy

For Ubuntu 22.04 LTS (still supportedm and I cannot update it for businness reasons):

https://packages.ubuntu.com/jammy/clang-14

[tool] Remove legacy argument -ignore-insert-conflict from `run-cla…

6c21029

…ng-tidy.py`

llvmbot added clang-tools-extra clang-tidy labels Feb 13, 2025

EugeneZelenko requested review from 5chmidti, HerrCai0907, PiotrZSL and carlosgalvezp February 13, 2025 15:45

PiotrZSL reviewed Feb 13, 2025

View reviewed changes

Optional

65adc60

loumalouomega changed the title ~~[tool] Remove legacy argument -ignore-insert-conflict from run-clang-tidy.py~~ [tool] Add support for -ignore-insert-conflict option in clang-apply-replacements Feb 14, 2025

Merge branch 'main' into main

1382569

carlosgalvezp reviewed Feb 14, 2025

View reviewed changes

clang-tools-extra/clang-tidy/tool/run-clang-tidy.py Outdated Show resolved Hide resolved

carlosgalvezp reviewed Feb 14, 2025

View reviewed changes

clang-tools-extra/clang-tidy/tool/run-clang-tidy.py Outdated Show resolved Hide resolved

carlosgalvezp approved these changes Feb 14, 2025

View reviewed changes

Suggestions

89e2b4d

loumalouomega changed the title ~~[tool] Add support for -ignore-insert-conflict option in clang-apply-replacements~~ [clang-tidy] Add support for -ignore-insert-conflict in run-clang-tidy.py Feb 14, 2025

carlosgalvezp reviewed Feb 14, 2025

View reviewed changes

clang-tools-extra/clang-tidy/tool/run-clang-tidy.py Show resolved Hide resolved

loumalouomega added 2 commits February 14, 2025 12:23

Merge branch 'main' into main

86c47be

[clang-tidy] Preserve current behaviour in run-clang-tidy.py

e8165bc

HerrCai0907 approved these changes Feb 14, 2025

View reviewed changes

Merge branch 'main' into main

80b0b79

nicovank reviewed Feb 15, 2025

View reviewed changes

carlosgalvezp reviewed Feb 17, 2025

View reviewed changes

clang-tools-extra/clang-tidy/tool/run-clang-tidy.py Outdated Show resolved Hide resolved

loumalouomega added 2 commits February 17, 2025 10:05

Merge branch 'main' into main

9251f16

[clang-tidy] Check for -ignore-insert-conflict support before using…

b1b5286

… it in `apply_fixes` in `run_clang_tidy.py`

loumalouomega changed the title ~~[clang-tidy] Add support for -ignore-insert-conflict in run-clang-tidy.py~~ [clang-tidy] Check for -ignore-insert-conflict support before using it in apply_fixes in run-clang-tidy.py Feb 17, 2025

Merge branch 'main' into main

2ad206f

Merge branch 'main' into main

f8ddede

loumalouomega closed this Feb 17, 2025

[clang-tidy] Check for -ignore-insert-conflict support before using it in apply_fixes in run-clang-tidy.py #127066

[clang-tidy] Check for -ignore-insert-conflict support before using it in apply_fixes in run-clang-tidy.py #127066

Uh oh!

Conversation

loumalouomega commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 13, 2025

Uh oh!

llvmbot commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary:

Uh oh!

HerrCai0907 commented Feb 13, 2025

Uh oh!

loumalouomega commented Feb 13, 2025

Uh oh!

PiotrZSL commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

loumalouomega commented Feb 13, 2025

Uh oh!

loumalouomega commented Feb 13, 2025

Uh oh!

loumalouomega commented Feb 14, 2025

Uh oh!

Uh oh!

Uh oh!

carlosgalvezp left a comment

Choose a reason for hiding this comment

Uh oh!

loumalouomega commented Feb 14, 2025

Uh oh!

Uh oh!

HerrCai0907 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loumalouomega commented Feb 15, 2025

Uh oh!

nicovank Feb 15, 2025

Choose a reason for hiding this comment

Uh oh!

carlosgalvezp Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

loumalouomega Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

HerrCai0907 commented Feb 16, 2025

Uh oh!

Uh oh!

whisperity commented Feb 17, 2025

Uh oh!

loumalouomega commented Feb 17, 2025

Uh oh!

carlosgalvezp commented Feb 17, 2025

Uh oh!

loumalouomega commented Feb 17, 2025

Uh oh!

whisperity commented Feb 19, 2025

Uh oh!

loumalouomega commented Feb 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

[clang-tidy] Check for `-ignore-insert-conflict` support before using it in `apply_fixes` in `run-clang-tidy.py` #127066

[clang-tidy] Check for `-ignore-insert-conflict` support before using it in `apply_fixes` in `run-clang-tidy.py` #127066

loumalouomega commented Feb 13, 2025 •

edited

Loading

llvmbot commented Feb 13, 2025 •

edited

Loading

PiotrZSL commented Feb 13, 2025 •

edited

Loading

github-actions bot commented Feb 14, 2025 •

edited

Loading