run-clang-tidy.py analyze unique files only

[serkazi]

Summary

The files selected from the "compile-commands.json" can potentially include duplicates. On a private repository, I had a factor of 3 duplication. After this change, the running time for the entire repo was slashed by half.

EDIT: Since "make_absolute()" is used, it is impossible that filenames such as "~/myfile.cpp" and "/home/user/myfile.cpp" are both present.

[kuhar]

I'm not sure if this diff is valid. The Revision Contents tab mentions a new file run-clang-tidy_new.py and I can't see the lines above or below the changed lines.

[kuhar]

run-clang-tidy_new.py

276 ↗

(On Diff #383783)

Would it be possible for the same file to still appear into two different paths? E.g., ~/myfile.cpp and /home/name/myfile.cpp?

[salman-javed-nz]

I don't think you intended to introduce a new file, did you?
Shouldn't this patch be applied to the existing run_clang_tidy.py?

See https://llvm.org/docs/Phabricator.html, the bit about "include as much context as possible", i.e. creating the patch with the -U999999 argument.

[serkazi]

Thanks all for the comments. Indeed, this is my first patch and I may have
mixed things up, and the patch should be applied to the existing
run-clang-tidy.py.
I don't know yet the answer to the question about ~/myfile.cpp and
/home/user/myfile.cpp; to answer that, I may need to consult the
"make_absolute_path" function's implementation from the context provided.

[salman-javed-nz]

Why does the compile-commands.json have duplicate entries in the first place? Would someone do that on purpose?

[serkazi]

In D112926#3102417, @salman-javed-nz wrote:

Why does the compile-commands.json have duplicate entries in the first place? Would someone do that on purpose?

Think of a large project where people -- for various reasons -- don't make full use of CMake targets... This way, the compile-commands would have multiple entries with "-c".

[serkazi]

I wonder colleagues what you would think is missing in this patch, or what
would be your concerns and comments

[salman-javed-nz]

It looks good to me. I don't have any more comments to add - it is a very small code change after all.
I have commit access and am happy to commit it on your behalf.

However, this would be my first time as a reviewer, and I don't want to be presumptuous and approve this before someone with more experience in the area has had a chance to look at it.

The rule of thumb is to wait a week before sending a ping. Then we'll see what we can do to get this through.

[serkazi]

In D112926#3108140, @salman-javed-nz wrote:

It looks good to me. I don't have any more comments to add - it is a very small code change after all.
I have commit access and am happy to commit it on your behalf.

However, this would be my first time as a reviewer, and I don't want to be presumptuous and approve this before someone with more experience in the area has had a chance to look at it.

The rule of thumb is to wait a week before sending a ping. Then we'll see what we can do to get this through.

Thank you. I leave it with you, then. And if in a week there is no response, I'll ping, as you've kindly suggested.

[kuhar]

I think this change is fine.

One could argue that having multiple files is an issue with tooling/build system in the first place, and we could consider printing a warning instead of silently fixing it up, but I don't think there's that much to gain from being pedantic here.

[lebedev.ri]

In D112926#3108745, @kuhar wrote:

I think this change is fine.

One could argue that having multiple files is an issue with tooling/build system in the first place,

No, why would that be a bug in itself? It may be a bug when they have the exact same externally-observable side-effects
(i.e. the output filename and/or the other options passed to the compiler).
The usual case where that could happen is when the source file has conditionally-compiled blocks,
and is compiled multiple times with different defines to produce different object files.

and we could consider printing a warning instead of silently fixing it up, but I don't think there's that much to gain from being pedantic here.

[serkazi]

In D112926#3108140, @salman-javed-nz wrote:

It looks good to me. I don't have any more comments to add - it is a very small code change after all.
I have commit access and am happy to commit it on your behalf.

However, this would be my first time as a reviewer, and I don't want to be presumptuous and approve this before someone with more experience in the area has had a chance to look at it.

The rule of thumb is to wait a week before sending a ping. Then we'll see what we can do to get this through.

This is now accepted, please feel free to merge on my behalf. Thanks.

[salman-javed-nz]

In D112926#3113206, @serkazi wrote:

This is now accepted, please feel free to merge on my behalf. Thanks.

What's the full name and email you want associated with your commit?

[serkazi]

In D112926#3113220, @salman-javed-nz wrote:

In D112926#3113206, @serkazi wrote:

This is now accepted, please feel free to merge on my behalf. Thanks.

What's the full name and email you want associated with your commit?

Let it be "Serikzhan Kazi [e-mail]"?

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

try:
     else:
       subprocess.check_call(invocation)
   except:
     print("Unable to run clang-tidy.", file=sys.stderr)
     sys.exit(1)
 
   # Load the database and extract all files.
   database = json.load(open(os.path.join(build_path, db_path)))
-  files = [make_absolute(entry['file'], entry['directory'])
-           for entry in database]
+  files = set([make_absolute(entry['file'], entry['directory'])
+           for entry in database])
 
   max_task = args.j
   if max_task == 0:
     max_task = multiprocessing.cpu_count()
 
   tmpdir = None
   if args.fix or (yaml and args.export_fixes):
     check_clang_apply_replacements_binary(args)