This is an archive of the discontinued LLVM Phabricator instance.

[Tooling] Use FixedCompilationDatabase when `compile_flags.txt` is found.
ClosedPublic

Authored by sammccall on Nov 8 2017, 6:15 AM.

Details

Summary

This is an alternative to JSONCompilationDatabase for simple projects that
don't use a build system such as CMake.
(You can also drop one in ~, to make your tools use e.g. C++11 by default)

There's no facility for varying flags per-source-file or per-machine.
Possibly this could be accommodated backwards-compatibly using cpp, but even if
not the simplicity seems worthwhile for the cases that are addressed.

Tested with clangd, works great! (requires clangd restart)

Event Timeline

sammccall created this revision.Nov 8 2017, 6:15 AM
klimek added inline comments.Nov 8 2017, 6:21 AM
include/clang/Tooling/CompilationDatabase.h
188

Perhaps loadFromTextFile?

lib/Tooling/CompilationDatabase.cpp
329

I assume we can't use ErrorOr as return value?

sammccall added inline comments.Nov 8 2017, 6:32 AM
include/clang/Tooling/CompilationDatabase.h
188

This mirrors JSONCompilationDatabase for consistency.
Happy to change this one, change both, or leave as-is. WDYT?

lib/Tooling/CompilationDatabase.cpp
329

Again this mirrors JSONCompilationDatabase. (And also CompilationDatabasePlugin, which is harder to change)
Change this one, change both, or leave as-is?

I can see that FixedCompilationDatabase does not set a working directory. Is this something we may want to have for compile_flags.txt or one would need to resort to compile_commands.json to get this?
E.g., I'd find it useful to add includes with paths relative to compile_flags.txt by putting -Iproject-dep/include there.

lib/Tooling/CompilationDatabase.cpp
339

Maybe use llvm:make_unique?

sammccall marked an inline comment as done.Nov 8 2017, 8:13 AM

I can see that FixedCompilationDatabase does not set a working directory. Is this something we may want to have for compile_flags.txt or one would need to resort to compile_commands.json to get this?
E.g., I'd find it useful to add includes with paths relative to compile_flags.txt by putting -Iproject-dep/include there.

Currently the working directory is always the parent of compile_flags.txt as you suggest.
Maybe this isn't great though - sometimes it might be important that the WD is the parent of the source file?

sammccall updated this revision to Diff 122100.Nov 8 2017, 8:21 AM

llvm::make_unique

Currently the working directory is always the parent of compile_flags.txt as you suggest.
Maybe this isn't great though - sometimes it might be important that the WD is the parent of the source file?

Parent of the source file seems rather inconvenient most of the time. Even when reading the contents of compile_flags.txt one would have to consider it the contexts of every source file.

I'm looking at CompilationDatabase.cpp:321 and don't see the CompileCommand::Directory being set at all. Maybe I'm missing something.

A few ideas for tests:

  1. Test relative include paths (-Irelpath/to/include) work.
  2. What happens if both compile_commands.json and compile_flags.txt are present? Should we test and document it? I'd expect the first one to take priority and the second one to come into play only if the input file is not found in the first one.

Currently the working directory is always the parent of compile_flags.txt as you suggest.
Maybe this isn't great though - sometimes it might be important that the WD is the parent of the source file?

Parent of the source file seems rather inconvenient most of the time. Even when reading the contents of compile_flags.txt one would have to consider it the contexts of every source file.

Understood, this is the best behavior I think. I'm just wondering whether it's a good enough heuristic to be silently applied.
e.g. IIUC, things like #include "sibling.h" won't look for the h file next to the cc file?

I'm looking at CompilationDatabase.cpp:321 and don't see the CompileCommand::Directory being set at all. Maybe I'm missing something.

Result is initialized with a copy of CompilerCommands, whose element is initialized with the directory passed to the constructor.

A few ideas for tests:

  1. Test relative include paths (-Irelpath/to/include) work.

Definitely will do this.

  1. What happens if both compile_commands.json and compile_flags.txt are present? Should we test and document it? I'd expect the first one to take priority and the second one to come into play only if the input file is not found in the first one.

Ideally yes... i'm not sure this is so easy with the registry mechanism though :-(
They'd have to be present at the same level, so I'm not sure this is a huge deal.

e.g. IIUC, things like #include "sibling.h" won't look for the h file next to the cc file?

Actually, this should work as quoted includes are always searched relative to the current file before looking in the include paths. No matter what the WD is.

Result is initialized with a copy of CompilerCommands, whose element is initialized with the directory passed to the constructor.

Missed that, thanks!

Ideally yes... i'm not sure this is so easy with the registry mechanism though :-(
They'd have to be present at the same level, so I'm not sure this is a huge deal.

Yeah, looks like this would require unifying FixedCompilationDatabase and JSONCompilationDatabase. This doesn't seem too crazy, though. "A class that handles reading compilation arguments from the filesystem." seems like a valid entity to me.

sammccall updated this revision to Diff 122206.Nov 9 2017, 12:53 AM

Add test verifying that working directory is compile_flags's parent.

e.g. IIUC, things like #include "sibling.h" won't look for the h file next to the cc file?

Actually, this should work as quoted includes are always searched relative to the current file before looking in the include paths. No matter what the WD is.

Right, thanks :-)

Ideally yes... i'm not sure this is so easy with the registry mechanism though :-(
They'd have to be present at the same level, so I'm not sure this is a huge deal.

Yeah, looks like this would require unifying FixedCompilationDatabase and JSONCompilationDatabase. This doesn't seem too crazy, though. "A class that handles reading compilation arguments from the filesystem." seems like a valid entity to me.

It's doable, but then you're fighting the infrastructure here. The same ambiguity can exist between any pair of CompilationDatabasePlugins (try dumping a compile_commands.json in a google source tree). If we want to make this an error or have well-defined priority, that should be handled by autoDetectFromDirectory I think.

(Happy to do that if you think it's important - personally I'm not convinced it will be an issue in practice)

ilya-biryukov accepted this revision.Nov 9 2017, 1:43 AM

It's doable, but then you're fighting the infrastructure here. The same ambiguity can exist between any pair of CompilationDatabasePlugins (try dumping a compile_commands.json in a google source tree). If we want to make this an error or have well-defined priority, that should be handled by autoDetectFromDirectory I think.

(Happy to do that if you think it's important - personally I'm not convinced it will be an issue in practice)

Yeah, I'm not sure if that's an issue either. Let's see whether anyone will be using both compile_flags.txt and compile_commands.json in the first place. We can always do it later.
Oh, and LGTM from my side!

This revision is now accepted and ready to land.Nov 9 2017, 1:43 AM
klimek accepted this revision.Nov 9 2017, 2:11 AM

lg

lib/Tooling/CompilationDatabase.cpp
329

I think we want to add a FIXME to change the interface.

sammccall updated this revision to Diff 122211.Nov 9 2017, 2:23 AM

Add fixme to upgrade out-param APIs.

This revision was automatically updated to reflect the committed changes.
JVApen added a subscriber: JVApen.Nov 9 2017, 11:29 AM

How does this new file know if it should handle it's flags as it does in clang.exe or clang-cl.exe?

How does this new file know if it should handle it's flags as it does in clang.exe or clang-cl.exe?

It doesn't set argv[0], so it's treated like clang by default.

I believe the flag --driver-mode={gcc,g++,cpp,cl} can be used to change this.
(I should add this to the brief doc)