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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
