Clang-Tidy Export Problem

[TheAhmad]

Summary

Hi.
Clang tidy has problem with compile command databases that do not use necassarily absolute file paths. This is common in many projects that use makefiles as the build system, whose databases are usually generated using Bear. Therefore, I reused some of the code of the in-place fix and revised it to generate a YAML export file. The diff is OK, except that the merge conflicts are not added, right now.
Thanks!

[Eugene.Zelenko]

ClangTidy.cpp

627

Please run Clang-format.

[Eugene.Zelenko]

ClangTidy.cpp

620

You could use auto, since it's iterator over container.

621

Curly braces are not necessary.

[TheAhmad]

Ran Clang Format.
Redundant Braces Removed.
Used auto.

[TheAhmad]

This comment was removed by TheAhmad.

[Eugene.Zelenko]

ClangTidy.cpp

612

You could use auto here, because type is in new statement.

614

Type is not obvious, so please don't use auto.

[JonasToth]

ClangTidy.cpp

612

Where will the delete happen for the FileManager? I think a unique_ptr would be a good idea, not?

you can use llvm::make_unique<>() to reduce the verbosity of C++11.

614

Please use a named cast for your intended purpose.

620

You could maybe use std::make_pair here.

647

make_pair too.

[TheAhmad]

Added auto at line 612.

[TheAhmad]

Used make_pair instead of constructor call.
Used unique_ptr.
Used named cast.

[lebedev.ri]

I'm not sure what this differential *actually* does.

That being said
https://clang.llvm.org/docs/JSONCompilationDatabase.html#format

directory: The working directory of the compilation. All paths specified in the command or file fields must be either absolute or relative to this directory.
file: The main translation unit source processed by this compilation step. This is used by tools as the key into the compilation database. There can be multiple command objects for the same file, for example if the same source file is compiled with different configurations.

Can you just make clang-tidy respect the directory value?
If directory is invalid, then it is not a valid Compilation Database to begin with..

[lebedev.ri]

Also, please upload the patches with full context (-U99999), and the patch is misformed, the ClangTidy.cpp isn't in the root directory.
Also, tests.

[JonasToth]

In D49890#1178103, @lebedev.ri wrote:

I'm not sure what this differential *actually* does.

This is a follow up from this discussion on the mailing list: http://clang-developers.42468.n3.nabble.com/Fwd-Running-Clang-Tidy-on-a-Large-Project-td4061261.html

[JonasToth]

ClangTidy.cpp

612

in this case you can use auto again, because the type of the unique_ptr is clear from the make_unique call.

[lebedev.ri]

In D49890#1178211, @JonasToth wrote:

In D49890#1178103, @lebedev.ri wrote:

I'm not sure what this differential *actually* does.

This is a follow up from this discussion on the mailing list: http://clang-developers.42468.n3.nabble.com/Fwd-Running-Clang-Tidy-on-a-Large-Project-td4061261.html

No, i saw that. But i'm still unable to parse the code.
It's a little bit hard to argue about the diff without the tests.

[TheAhmad]

The most comprehensive fix is to support all possible valid compilation databases. The current fix is the fastest one.

Can you just make clang-tidy respect the directory value?

This is exactly what I did. I prepended the directory value to the file value and stored the result in the exported file.
There was also problem with merges to the same location. What I did is similar to the code for inplace fix (starting at line 133 of ClangTidy.cpp), except that I have not added the code for merge conflicts. The ultimate fix requires adding a new function and moving the shared code there. Then inplace fix and export will use that function.

ClangTidy.cpp

614

Hi, Eugene. Why line 352 uses auto?

[TheAhmad]

Also, tests.

What should I provide for tests?

[JonasToth]

ClangTidy.cpp

614

He means line 615 (InitialWorkingDir). The type of the variable can not be deduced from reading the code.

The rule is, to write the type once. E.g. llvm::make_unique<MyType>(args) makes it clear, that the type is MyType, so you can use auto for the variable.
This is not the case for InitialWorkingDir.

[JonasToth]

Actually, maybe this patch is relevant for clang-tooling itself (where the compilation-database stuff actually is). @alexfh what is your opinion on that?

llvm/tools/clang/unittests/Tooling/CompilationDatabaseTest.cpp This is the path for the unittests there.
llvm/tools/clang/tools/extra/unittests/clang-tidy This is the path for the clang-tidy unittests, where the tests for this patch should land.

[TheAhmad]

auto used at line 612.
auto removed at line 614.

[alexfh]

Could you describe the specific problem you're solving and provide an example? As mentioned by others, a test would be very welcome as well.

[TheAhmad]

In D49890#1182556, @alexfh wrote:

Could you describe the specific problem you're solving and provide an example? As mentioned by others, a test would be very welcome as well.

Sorry for so much delay, @alexfh. I didn't see your comment. I will describe in detail:
I wanted to do a source to source transformation on MPlayer-1.3.0 source code. The transformation may require modification of many files and possibly repeated modifications in the headers files included in multiple .c files. Therefore, the changes should be serialized for each translation unit and stored in a YAML file. At the end, clang-apply-replacements will be called and transform the entire source code.
The problem is that clang-tidy expects a limited format for the compilation database. This is the format typically used when the build system generating the compilation database is CMAKE. But MPlayer uses Makefile. Therefore, I had to use an external database generator, Bear. In this case, the contents of the YAML files are OK. But it is not what is expected by clang-tidy. clang-tidy requires every file path to be absolute, even the header files.
The problem (i.e., using relative paths) only arises when the fixes are exported. Not when they are applied in-place. I reused some of the code for the in-place case and did some modifications to it. The code is OK, at least for my case with MPlayer. A small change is still needed to support merge conflicts which can be brought from the in-place fix stuff. It seems that at the end the commanlities of the two cases should be put in a function. Then this function can be called from both places (i.e., the in-place fix and the export fix).
I am new to Clang and do not know what is needed for tests. I am looking forward to your reply.
Regards.

ClangTidy.cpp

614

Right. I agree. So line 352 should not use auto either.

[alexfh]

In D49890#1200061, @TheAhmad wrote:

In D49890#1182556, @alexfh wrote:

Could you describe the specific problem you're solving and provide an example? As mentioned by others, a test would be very welcome as well.

Sorry for so much delay, @alexfh. I didn't see your comment. I will describe in detail:
I wanted to do a source to source transformation on MPlayer-1.3.0 source code. The transformation may require modification of many files and possibly repeated modifications in the headers files included in multiple .c files. Therefore, the changes should be serialized for each translation unit and stored in a YAML file. At the end, clang-apply-replacements will be called and transform the entire source code.
The problem is that clang-tidy expects a limited format for the compilation database. This is the format typically used when the build system generating the compilation database is CMAKE. But MPlayer uses Makefile. Therefore, I had to use an external database generator, Bear. In this case, the contents of the YAML files are OK. But it is not what is expected by clang-tidy. clang-tidy requires every file path to be absolute, even the header files.
The problem (i.e., using relative paths) only arises when the fixes are exported. Not when they are applied in-place. I reused some of the code for the in-place case and did some modifications to it. The code is OK, at least for my case with MPlayer. A small change is still needed to support merge conflicts which can be brought from the in-place fix stuff. It seems that at the end the commanlities of the two cases should be put in a function. Then this function can be called from both places (i.e., the in-place fix and the export fix).
I am new to Clang and do not know what is needed for tests. I am looking forward to your reply.
Regards.

Sorry for missing your reply (vacation, travels - and a lot of mail got buried under other mail). Feel free to ping regularly, if you don't get a response.

The solution you propose seems reasonable, but 1. we need a test, 2. I would like to better understand where relative paths are coming from (directory? include search paths in the compilation command?). Could you upload an example of a problematic compilation database?

ClangTidy.cpp

612

Should we get the file manager from the SourceManager as in handleErrors?

614

Agree. Line 352 should use explicit type.

632

I'd use try_emplace instead.

[TheAhmad]

Hi @alexfh,
I have attached the compilation database for MPlayer-1.3.0.

compile_commands.json2 MBDownload

As can be seen, the paths are relative. To answer your comments I need more time. I do not remember the exact details. I will prepare a test as soon as possible. I did not use the test infrastructure, before. Therefore, I should learn about it.
Regards.

[kfunk]

Is someone willing to pick up this patch again? It's still an issue.

[ngzhian]

I found this patch while searching for a similar problem I faced while tryin to use clang-tidy to fix some warnings in V8 (Chrome's JavaScript engine).
The setup for our compilation is as such:

• basedir
  • src/
  • out/ -arch1/
    • • .o files, generated files
      • compile_commands.json
    • arch2/

We use ninja to build, so usually with a command like ninja -C out/arch1, this means (roughly) cd into out/arch1 and build.

Given this, I have an alternative suggestion for a fix. In run-clang-tidy.py apply_fixes, we call the clang-apply-replacement with cwd set to build_path, like so:

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 1eb13529575..cde9e3671af 100755
--- a/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ b/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -150,7 +150,7 @@ def apply_fixes(args, tmpdir):
   if args.style:
     invocation.append('-style=' + args.style)
   invocation.append(tmpdir)
-  subprocess.call(invocation)
+  subprocess.call(invocation, cwd=args.build_path)

def run_tidy(args, tmpdir, build_path, queue, lock, failed_files):

I don't know the internals of clang-tidy enough to do this, but it works for me. And it seems reasonable, since the Replacement FilePath in the yaml file consumed by clang-apply-replacement was generated as a path relative to the build_path anyway. For now this is a sufficient hack for my use case (tried with with just 1 data point as a test), and it will be good to get feedback if this fix is correct. Thanks!

[jwillemsen]

I am trying to use run-clang-tidy on ACE/TAO (see https://github.com/DOCGroup/ACE_TAO) but it doesn't work, also the error reported here. We are using GNU make using our own set of make rules. I used bear to generate the compile_commands.json

When only running modernize-use-override I get a yaml file of 110167 lines but that can't also be applied using clang-apply-replacements, same errors.

The last proposed change to run-clang-tidy.py doesn't work for me.

ClangTidy.cpp

                         raw_ostream &OS) {
   TranslationUnitDiagnostics TUD;
   TUD.MainSourceFile = MainFilePath;
-  for (const auto &Error : Errors) {
-    tooling::Diagnostic Diag = Error;
+
+  FileManager *Files = new FileManager(FileSystemOptions());
+  vfs::FileSystem &FileSystem = *Files->getVirtualFileSystem();
+  auto InitialWorkingDir = FileSystem.getCurrentWorkingDirectory();
+  if (!InitialWorkingDir)
+    llvm::report_fatal_error("Cannot get current working path.");
+
+  llvm::StringMap<Replacements> FileReplacements;
+  llvm::StringMap<ClangTidyError> SingleErrors;
+  for (const ClangTidyError &Error : Errors) {
+    if (!Error.BuildDirectory.empty()) {
+      FileSystem.setCurrentWorkingDirectory(Error.BuildDirectory);
+    }
+
+    SmallString<128> ErrorAbsoluteFilePath = (StringRef)Error.Message.FilePath;
+    Files->makeAbsolutePath(ErrorAbsoluteFilePath);
+    if (SingleErrors.find(ErrorAbsoluteFilePath) == SingleErrors.end())
+    {
+      ClangTidyError AbsoluteError = Error;
+      AbsoluteError.Message.FilePath = ErrorAbsoluteFilePath.str();
+      AbsoluteError.Fix.clear();
+      SingleErrors.insert(std::pair<StringRef,ClangTidyError>(ErrorAbsoluteFilePath, AbsoluteError));
+    }
+  
+    for (const auto &FileAndReplacements : Error.Fix) {
+      for (const auto &Repl : FileAndReplacements.second) {
+        SmallString<128> FixAbsoluteFilePath = Repl.getFilePath();
+        Files->makeAbsolutePath(FixAbsoluteFilePath);
+
+        tooling::Replacement R(FixAbsoluteFilePath, Repl.getOffset(),
+                                      Repl.getLength(),
+                                      Repl.getReplacementText());
+
+        FileReplacements[R.getFilePath()].add(R);
+      }
+    }
+
+    FileSystem.setCurrentWorkingDirectory(InitialWorkingDir.get());
+  }
+
+  for (const auto &FileAndReplacements : FileReplacements) {
+    StringRef File = FileAndReplacements.first();
+    Replacements Repls = FileAndReplacements.second;
+
+    ClangTidyError  AbsoluteError = SingleErrors.find(File)->second;   
+    if (SingleErrors.find(File) == SingleErrors.end())
+    {
+      llvm::report_fatal_error("Cannot find the containing ClangTidyError.");
+    }
+    AbsoluteError.Fix.insert(std::pair<StringRef,Replacements>(File,Repls));
+
+    tooling::Diagnostic Diag = AbsoluteError;
     TUD.Diagnostics.insert(TUD.Diagnostics.end(), Diag);
   }