Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guess compile commands in irony-server. #270

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Hylen
Copy link
Contributor

@Hylen Hylen commented Dec 30, 2015

Using rapidjson to read the database, and cache the database between
subsequent reads. For guessing commands, path operations from boost
filesystem are used. If boost filesystem isn't detected on the system,
the database is disabled.

Using rapidjson to read the database, and cache the database between
subsequent reads. For guessing commands, path operations from boost
filesystem are used. If boost filesystem isn't detected on the system,
the database is disabled.
@Sarcasm
Copy link
Owner

Sarcasm commented Jan 14, 2016

One question first.
I really don't like to add dependencies to irony-mode because it will not be as easy as before to use. irony-mode is not really installable by distros right now so adding dependencies is more work to the end user.

Anyway, here I notice that we have 2 new dependencies, why not use only Boost which provide a JSON parser?

Also, which the big file: server/test/elisp/compile_commands.json ?
I'm not against unit test but they should usually be minimal and test one thing, when we detect a bug we create a new test with the faulty code. For example with_quote.json seems at least to indicate what is being tested specifically. Why is it so long though?

This thing

[
{
  "directory": "/home/user/dev/llvm-build/tools/llvm-config",
  "command": "/usr/bin/c++   -DCMAKE_CFG_INTDIR=\\\".\\\" -DGTEST_HAS_RTTI=0 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS  -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wno-maybe-uninitialized -Wno-comment -std=c++11 -g -I/home/user/dev/llvm-build/tools/llvm-config -I/home/user/dev/llvm/tools/llvm-config -I/home/user/dev/llvm-build/include -I/home/user/dev/llvm/include    -fno-exceptions -fno-rtti -o CMakeFiles/llvm-config.dir/llvm-config.cpp.o -c /home/user/dev/llvm/tools/llvm-config/llvm-config.cpp",
  "file": "/home/user/dev/llvm/tools/llvm-config/llvm-config.cpp"
}
]

tests the same thing as this thing ?

[
{
  "directory": "/",
  "command": "clang -DBAR=\\\".\\\" -c bar.cpp -o bar.o",
  "file": "/bar.cpp"
}
]

For the caching part I'm okay with the idea, for the guessing part I'm not sure. I think I would prefer the user to have the possibilities to document the mapping or have an indexer tool giving this information. The strategy you used might be an interesting default.

@Hylen
Copy link
Contributor Author

Hylen commented Jan 25, 2016

One question first.
I really don't like to add dependencies to irony-mode because it will not be as easy as before to use. irony-mode is not really installable by distros right now so adding dependencies is more work to the end user.

Anyway, here I notice that we have 2 new dependencies, why not use only Boost which provide a JSON parser?

Since we'll be making boost an optional dependency this change shouldn't make Irony any more complicated to use, only nicer for people with boost installed. At least that's how I see it.

I didn't know boost shipped a JSON parser. I will update the pull request with it when I have the extra time (which is in short supply now).

Also, which the big file: server/test/elisp/compile_commands.json ?
I'm not against unit test but they should usually be minimal and test one thing, when we detect a bug we create a new test with the faulty code. For example with_quote.json seems at least to indicate what is being tested specifically. Why is it so long though?

I added a quite large JSON file because I wrote a test that checked if irony-cdb-json and irony-cdb-server returned the same result. We can remove this test if you want, but in my opinion it's always better to test more.

For the caching part I'm okay with the idea, for the guessing part I'm not sure. I think I would prefer the user to have the possibilities to document the mapping or have an indexer tool giving this information. The strategy you used might be an interesting default.

Yes, I agree we could do better than this but it also comes down to speed. The user shouldn't notice the time it takes to load the database when opening a new file. I've got some ideas on how to parse the sources to get more accurate header compilation options, but it might make things slower. Also, this algorithm is very accurate, I've been using it for months and haven't noticed any problem with it.

@dvzubarev
Copy link

Hi, thanks for your work. This patch was very helpful to me. It seems you may
relax the required version of boost. I had boost 1.54 installed, so I changed the minimal version to 1.54 in cmakelists.txt and compile. Everything works fine for me.

@Hylen
Copy link
Contributor Author

Hylen commented Mar 12, 2016

@dvzubarev Glad to hear that you've had use of it. I've opened a new pull request rebased on current master only requiring 1.54.

@qdot
Copy link

qdot commented Jul 8, 2016

I also got this patch with working with reasonable speed on top of gecko (the firefox web engine), which produces a 5.4mb compile_commands.json file.

Looks like there's been some changes to irony-mode since the last revision that break things if you try to rebase (irony only passes the compilation db directory instead of the whole file path, which makes boost confusingly throw an underflow error since the irony mode server doesn't check the path to make sure it's a valid file), so there needs to be another patch revision to bring it up to date.

@Sarcasm
Copy link
Owner

Sarcasm commented Jul 23, 2016

Hi @qdot,

Out of curiosity, I took a look at Firefox codebase to see how to generate a compilation database and find out a reasonable way to get header compile options.

I actually found that there is a command in the Firefox build system that should do what you want without the need for irony-server's guessing, any reason not to use it?

~/dev/src/mozilla/firefox-47.0$ ./mach compileflags view/nsView.h                    
-I/home/user/dev/src/mozilla/firefox-47.0/obj-x86_64-unknown-linux-gnu/dist/stl_wrappers
-I/home/user/dev/src/mozilla/firefox-47.0/obj-x86_64-unknown-linux-gnu/dist/system_wrappers
-include /home/user/dev/src/mozilla/firefox-47.0/config/gcc_hidden.h -DNDEBUG=1
-DTRIMMED=1 -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL
-I/home/user/dev/src/mozilla/firefox-47.0/view
-I/home/user/dev/src/mozilla/firefox-47.0/obj-x86_64-unknown-linux-gnu/view
-I/home/user/dev/src/mozilla/firefox-47.0/obj-x86_64-unknown-linux-gnu/dist/include
-I/home/user/dev/src/mozilla/firefox-47.0/obj-x86_64-unknown-linux-gnu/dist/include/nspr
-I/home/user/dev/src/mozilla/firefox-47.0/obj-x86_64-unknown-linux-gnu/dist/include/nss
-fPIC -DMOZILLA_CLIENT -include
/home/user/dev/src/mozilla/firefox-47.0/obj-x86_64-unknown-linux-gnu/mozilla-config.h
-Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual
-Wpointer-arith -Wsign-compare -Wtype-limits -Wwrite-strings -Wc++14-compat
-Wunreachable-code -Wno-invalid-offsetof -Wno-error=maybe-uninitialized
-Wno-error=deprecated-declarations -Wno-error=array-bounds -fno-exceptions
-fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x
-pthread -pipe -g -freorder-blocks -Os -fomit-frame-pointer -Wshadow
-I/home/user/dev/src/mozilla/firefox-47.0/obj-x86_64-unknown-linux-gnu/dist/include/cairo

Seems like they have these few handy commands:

  clang-complete        Generate a .clang_complete file.
  compileflags          Display the compilation flags for a given source file

@Hylen: I don't think I will ever accept the server guessing code for irony-server.

I think the compile options problem is useful to other things than just irony-mode, also I think this may be something quite involved by itself that can grow bigger than the actual irony-server.

It shares almost nothing from libclang, so I think this could be made a standalone tool. It would be easy to detect if the tool is available and use it in an irony-cdb backend (this I would not be opposed, I might actually use it).

FYI, I have started working on a python tool to manipulate compilation databases:

It's nothing too interesting for now but I implemented a command that generate a header compilation database from a compile_commands.json.

For example, for irony-mode, one could do proceed as follow to generate a header compilation database:

mkdir build
cd build
cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=ON ../server
compdb headerdb -p .

FYI, in this tool I have taken the following approach for now for the guessing:

  • parse all files a of compilation database, resolve the headers (file #included). It's slow if the project is big. Takes ~11 seconds for me on llvm. But it's once per build system changes, not once per file. The method used should not have false positives since includes are supposedly resolved like the compiler would (theorically, in practice I doubt it but nothing that can't be fixed).
  • I have a scoring like you do but different. I went on scoring on "subwords" (CamelCase, lowercase_underscore) in the filename.

@qdot
Copy link

qdot commented Jul 23, 2016

@Sarcasm I'd tried using the .clang_complete method before, it'd never really worked for me, but I could give it another shot. I wasn't really looking for the command guessing portions of the patched irony-server anyways, more loading compile_commands.json in C++. Since it's a huge file, loading and parsing it in elisp was what was making everything slow (I profiled it in emacs and the json parsing functions were something like 80-90% of the time spent during file load). That's really the only thing I need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants