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

Per file compile flags and cwd #31

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Per file compile flags and cwd #31

wants to merge 7 commits into from

Conversation

nico
Copy link
Contributor

@nico nico commented Feb 6, 2011

Incorporate the feedback from #29

From what I understand, libclang will either soon support or does already support passing in a current working directory instead of using getcwd(). Once that's accessible from python, that should be used instead.

If you guys think this patch is good as-is, I will add support for this to the non-libclang codepath as well.

@nico
Copy link
Contributor Author

nico commented Feb 6, 2011

Feedback addressed.

I think libclang will support passing in an explicit cwd soon; once that's accessible from python, we should use that instead of temporarily changing cwd.

If you guys think this looks good, I'll add support for this to the non-libclang code path too.

@nico
Copy link
Contributor Author

nico commented Feb 8, 2011

Ping?

import re
import time
import vim

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change needed? In case it is not I would prefer to not change it.

@Silex
Copy link
Collaborator

Silex commented May 1, 2011

Where are we about this pull request? Seems there's a lot of unresolved questions... is the code here something Rip-Rip should consider pulling? What is left to be done?

Thanks! :)

@nico
Copy link
Contributor Author

nico commented May 2, 2011

Hi,

On 01.05.2011, at 13:54, Silex wrote:

Where are we about this pull request? Seems there's a lot of unresolved questions... is the code here something Rip-Rip should consider pulling? What is left to be done?

I'll get to it eventually. Things are hectic right now, but I'd like to get some form of this in soonish.

One thing that's making me reconsider things is the addition of the clang-check binary to the clang tree as of last week. But since that doesn't do code completion (and no caching, so even if it did it would be slow), I think I still need this patch.

Nico

@Silex
Copy link
Collaborator

Silex commented May 2, 2011

One thing that's making me reconsider things is the addition of the clang-check binary to the clang tree as of last week. But since that doesn't do code completion (and no caching, so even if it did it would be slow), I think I still need this patch.

I must admit I'm kinda lost about this comment and the purpose of this pull request. As I understand it, your goal is to make it so you can give different compiler flags per file, for the case when some of the files are compiled with extra defines or stuffs like that.

There are two things unclear to me:

  1. Why do you care about getcwd() and/or the current directory? it's probably obvious but I can't figure it out :)
  2. Can you explain or point to somewhere that explains what you mean about the "clang-check binary to the clang tree"?

Thanks!
Silex

" return {}
" endif
" endfu
"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is too much limiting. Let's say that I have a directory with a.cpp and b.cpp files inside it, and for some reason I've specified two separate sets of build options in my Makefile. This approach would make it impossible to set the correct options for both of these files.

Also, designing clang_per_file_user_options this way causes another (IMO more major) problem. Let's say that you're working on a very large project with tons of directories. This would practically mean that you have to store the build options twice, once in your build system (Makefiles or whatnot) and once again in another db accessible by clang_per_file_user_options (as in https://gist.github.com/813631). However, if clang_per_file_user_options way designed to take the file name instead, it could do something like:

make source.cpp CC=echo CXX=echo

to make the build system print the correct command line for the file in question. This approach would address both of the above problems. What do you think?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, looking closer into this, I think the clang_per_file_user_options implementation in https://gist.github.com/813631 might have tricked me, although I'm still not sure if that would provide the flexibility of getting the build options from the build system itself or not...

@Silex
Copy link
Collaborator

Silex commented May 24, 2011

make source.cpp CC=echo CXX=echo

I have a similar idea (originally from gccsense) in order to automatically create the .clang_complete files, I could imagine extending it in order to create .foobar_clang_user_options as well.

@nico nico closed this May 1, 2012
@nico
Copy link
Contributor Author

nico commented Aug 31, 2012

I'd like to reopen this. In Chormium, we now use the ninja build system, which can hand out compile flags for given files very quickly. So if something like this patch is accepted, clang_complete could mostly Just Work without having to have hacks to make the build system write magic dotfiles to various locactions.

This would look like so (with less hard-coded directories in practice, of course):

let g:clang_library_path = '/Users/thakis/src/llvm-svn/Release+Asserts/lib'
let g:clang_use_library = 1

let g:cr_root = '/Volumes/MacintoshHD2/src/chrome-git/src/'

" Return the compile flags that should be used for file |path| by
" querying the build database.
fu! g:clang_per_file_user_options(path)
  if a:path !~? g:cr_root
    return {}
  endif

  let l:path = a:path[strlen(g:cr_root):]

  let l:flags = system('ninja -C out/Release -t commands | grep ' . l:path)

  if l:flags == ''
    echo 'Could not find flags for file '.l:path
    return {}
  endif

  " Filter out options that -cc1 doesn't understand.
  let l:all_flags_list = split(l:flags)
  let l:cc1_flags = []
  let l:i = 0
  let l:e = len(l:all_flags_list)
  while l:i < l:e
    let arg = l:all_flags_list[i]
    if arg =~# "^-[IDFfmOW]"
      call add(l:cc1_flags, arg)
    endif

    if arg == '-isysroot'
      call add(l:cc1_flags, arg)
      call add(l:cc1_flags, l:all_flags_list[i + 1])
      let i += 1
    endif
    if arg == '-arch'
      call add(l:cc1_flags, arg)
      call add(l:cc1_flags, l:all_flags_list[i + 1])
      let i += 1
    endif

    let l:i += 1
  endwhile

  let l:cwd = g:cr_root . 'out/Release'
  return { 'flags': ' '.join(l:cc1_flags), 'cwd': l:cwd }
endfu

If that sounds fine to folks, I'll try to rebase this patch to trunk.

@nico nico reopened this Aug 31, 2012
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