[PATCH] D49890: Clang-Tidy Export Problem

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

In D49890#1200061 <https://reviews.llvm.org/D49890#1200061>, @TheAhmad wrote:

> In D49890#1182556 <https://reviews.llvm.org/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?



================
Comment at: ClangTidy.cpp:614
+  vfs::FileSystem &FileSystem = *Files->getVirtualFileSystem();
+  auto InitialWorkingDir = FileSystem.getCurrentWorkingDirectory();
+  if (!InitialWorkingDir)
----------------
TheAhmad wrote:
> JonasToth wrote:
> > TheAhmad wrote:
> > > Eugene.Zelenko wrote:
> > > > Type is not obvious, so please don't use auto.
> > > Hi, Eugene. Why line 352 uses auto?
> > 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`.
> Right. I agree. So `line 352` should not use `auto` either.
Agree. Line 352 should use explicit type.


================
Comment at: ClangTidy.cpp:612
+
+  auto Files = make_unique<FileManager>(FileSystemOptions());
+  vfs::FileSystem &FileSystem = *Files->getVirtualFileSystem();
----------------
Should we get the file manager from the SourceManager as in handleErrors?


================
Comment at: ClangTidy.cpp:632
+      AbsoluteError.Fix.clear();
+      SingleErrors.insert(std::make_pair(ErrorAbsoluteFilePath, AbsoluteError));
+    }
----------------
I'd use `try_emplace` instead.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D49890/new/

https://reviews.llvm.org/D49890