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

Support for compile_flags.txt? #110

Closed
oblitum opened this issue Nov 8, 2018 · 18 comments
Closed

Support for compile_flags.txt? #110

oblitum opened this issue Nov 8, 2018 · 18 comments
Labels
wontfix This will not be worked on

Comments

@oblitum
Copy link

oblitum commented Nov 8, 2018

I've just learned of the project and couldn't find whether it supports compile_flags.txt like clangd does.

@MaskRay
Copy link
Owner

MaskRay commented Nov 8, 2018

ccls does not read compile_flags.txt (clang::tooling::FixedCompilationDatabase).

It reads .ccls or compile_commands.json https://github.com/MaskRay/ccls/wiki/Getting-started#project-setup

@oblitum
Copy link
Author

oblitum commented Nov 8, 2018

I know, I just asked out of conformance with clang tools. I'll close it as it seems there's no interest(?).

@oblitum oblitum closed this as completed Nov 8, 2018
@MaskRay
Copy link
Owner

MaskRay commented Nov 8, 2018

It is more than "there's no interest" :)

compile_flags.txt was added last November (D39799). It uses clang-tool as the compiler driver. I feel it might be better to give the user more control. You can change the compiler driver to clang-cl or aarch64-linux-gnu-clang++ clang++, and see what happens.

%cpp %c are useful for language specific options.

FixedCompilationDatabase::
FixedCompilationDatabase(Twine Directory, ArrayRef<std::string> CommandLine) {
  std::vector<std::string> ToolCommandLine(1, "clang-tool");
  ToolCommandLine.insert(ToolCommandLine.end(),
                         CommandLine.begin(), CommandLine.end());
  CompileCommands.emplace_back(Directory, StringRef(),
                               std::move(ToolCommandLine),
                               StringRef());
}

@oblitum
Copy link
Author

oblitum commented Nov 10, 2018

@MaskRay can you help me on how should I put this compile_flags.txt in .ccls format?

-Wall
-Wextra
-pedantic
-std=c++17
-stdlib=libc++
-isystem /usr/local/include
-isystem /usr/lib/clang/7.0.0/include
-isystem /usr/include
-isystem /usr/include/c++/v1

I've always used this with YouCompleteMe and clangd, they work fine, but it seems I'm having trouble with the -isystem flags with ccls due to whitespace(?). Note that I've always had to be explicit about system include paths because libclang in general don't do (at least didn't) it by default.

@MaskRay
Copy link
Owner

MaskRay commented Nov 10, 2018

-isystem
/usr/local/include

@oblitum
Copy link
Author

oblitum commented Nov 10, 2018

@MaskRay I have indeed tried that before too, but then indexing didn't work quite right, I've checked the log and it was indexing much less files in /usr than if I use -I/usr/local/include, etc. Due to that, for example, I was allowed to complete cout from std::, but std::cout.<something> came empty, no members, while it's not empty when I use -I/usr/local/include.

@MaskRay
Copy link
Owner

MaskRay commented Nov 10, 2018

Sorry I didn't read your comment carefully. The .ccls format:

clang++
-Wall
-Wextra
-pedantic
-std=c++17
-stdlib=libc++
-isystem/usr/local/include
-isystem/usr/lib/clang/7.0.0/include
-isystem/usr/include
-isystem/usr/include/c++/v1

But I'd say /usr/lib/clang/7.0.0/include /usr/include/c++/v1 /usr/lib/clang/7.0.0/include are really redundant.

@oblitum
Copy link
Author

oblitum commented Nov 10, 2018

is it right without the % before clang++? Will try it here.

@MaskRay
Copy link
Owner

MaskRay commented Nov 10, 2018

Read wiki. I use %clang to mean either clang or clang++

@oblitum
Copy link
Author

oblitum commented Nov 10, 2018

But I'd say /usr/lib/clang/7.0.0/include /usr/include/c++/v1 /usr/lib/clang/7.0.0/include are really redundant.

I do that because I'm used to specify include locations when using libclang tools, the reason is here: ycm-core/YouCompleteMe#303 (comment). Libclang may have changed that behavior already, I don't know, and ccls may act completely different than that?

I think I will just remove them anyway.

But still, I found it strange that with these flags, completion and indexing doesn't work right, try completing std::cout.<something>, it doesn't work, but std::<something> does.

@oblitum
Copy link
Author

oblitum commented Nov 10, 2018

Read wiki. I use %clang to mean either clang or clang++

Ah, I did read, but only understood it now.

@oblitum
Copy link
Author

oblitum commented Nov 10, 2018

Thanks for the help. Sadly, no vim or neovim general LSP implementation is doing signatureHelp right. There's one libclang plugin that heard my suggestion and implemented it OK:

Sadly it's clang specific, and currently I can't even build it.

@MaskRay
Copy link
Owner

MaskRay commented Nov 10, 2018

I am unclear about the history as I started hacking clang last December.

ccls invokes clang::createInvocationFromCommandLine
cquery calls some libclang API like clang_indexSourceFileFullArgv
ycmd/cpp/ycm/ClangCompleter/TranslationUnit.cpp invokes clang_codeCompleteAt which calls clang::ASTUnit::CodeComplete with some pre-created CompilerInvocation created by clang::createInvocationFromCommandLine

clang::createInvocationFromCommandLine runs clangDriver to get system search directories. -isystem/usr/include is mostly redundant (it still has some effect: it changes the search order as you can bserve in the output of clang -E -v -xc++ /dev/null, but -externc-isystem/usr/include should probably be a total redundancy)

To explore libclang API, you may experiment with c-index-test -index-file a.cc -resource-dir ... [other options]. You may notice that it does not require extra -isystem.

Sadly, no vim or neovim general LSP implementation is doing signatureHelp right.

It can be the case.. I watch clangd development closely and I think both ccls and clang's server-side signatureHelp implementation should be good, but the clients may be at fault. Is you complaint similar to my emacs-lsp/lsp-mode#422 ?

@oblitum
Copy link
Author

oblitum commented Nov 10, 2018

This should have changed at some point I think. I'm an oldie on clang but has been some time I don't touch the codebase to know when that changed. I've implemented this and committed to clang, which today is used by signatureHelp implementers, at least the ones that do it.

@MaskRay
Copy link
Owner

MaskRay commented Nov 10, 2018

Thank you for rC26670 and its friends!

So it seems you may have landed it without a review? I think no one is focusing on libclang now and lots of contribution may go through a really long process...

My clang journey started with D41575, when I was still hacking on cquery.

I've found libclang (clang-c) quite cumbersome to use:

  • The required ASTUnit used by libclang may be a bit inefficient for many use cases
  • Its separation of definition/reference callbacks causes lots of code duplication
  • lots of mapping from C++ to C is just awkward.

@oblitum
Copy link
Author

oblitum commented Nov 10, 2018

No no, there was a review for sure: https://reviews.llvm.org/D6880.

@oblitum
Copy link
Author

oblitum commented Nov 10, 2018

I know, there's much stuff in any codebase that's not pretty, and anything clang is no exception :)

@MaskRay
Copy link
Owner

MaskRay commented Nov 10, 2018

For clang::CodeCompleteConsumer::ProcessOverloadCandidates, I forgot to mention that the author of irony-mode contributed cquery 391

The implementation is a bit different now in ccls and the optional argument part is split into a separate initialization option:

completion.detailedLabel
completion.duplicateOptional

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants