Permalink
Browse files

Effectively take g:clang_user_options into account at init time.

Also, instead of counting the number of diagnostics to see if the
builtin headers are found, return False only if an error was
encountered.
  • Loading branch information...
Rip-Rip committed Jan 31, 2013
1 parent 12bda7b commit 478611d83556bc2537157f1c5c4b4bd05ea2ad8d
Showing with 20 additions and 11 deletions.
  1. +20 −11 plugin/libclang.py
View
@@ -13,15 +13,20 @@ def canFindBuiltinHeaders(index, args = []):
flags = 0
currentFile = ("test.c", '#include "stddef.h"')
tu = index.parse("test.c", args, [currentFile], flags)
- return len(tu.diagnostics) == 0
+ for diag in tu.diagnostics:
+ if diag.severity >= diag.Error:
+ # Maybe we should print the diagnostic? This would speedup the
+ # time to fix a issue, both for the user and for us.
+ return False
+ return True
# Derive path to clang builtin headers.
#
# This function tries to derive a path to clang's builtin header files. We are
# just guessing, but the guess is very educated. In fact, we should be right
# for all manual installations (the ones where the builtin header path problem
# is very common).
-def getBuiltinHeaderPath(library_path):
+def getBuiltinHeaderPath(library_path, user_options):
path = library_path + "/../lib/clang"
try:
files = os.listdir(path)
@@ -30,8 +35,8 @@ def getBuiltinHeaderPath(library_path):
files = sorted(files)
path = path + "/" + files[-1] + "/include/"
- arg = "-I" + path
- if canFindBuiltinHeaders(index, [arg]):
+ arg = ["-I" + path] + user_options
+ if canFindBuiltinHeaders(index, arg):
return path
return None
@@ -58,10 +63,19 @@ def initClangComplete(clang_complete_flags, clang_compilation_database, \
print "Are you sure '%s' contains libclang?" % library_path
return 0
+ global compilation_database
+ if clang_compilation_database != '':
+ compilation_database = CompilationDatabase.fromDirectory(clang_compilation_database)
+ else:
+ compilation_database = None
+
+
global builtinHeaderPath
builtinHeaderPath = None
- if not canFindBuiltinHeaders(index):
- builtinHeaderPath = getBuiltinHeaderPath(library_path)
+ params = getCompileParams(vim.current.buffer.name)
+
+ if not canFindBuiltinHeaders(index, params["args"]):

This comment has been minimized.

Show comment Hide comment
@tobig

tobig Feb 1, 2013

Collaborator

This is the part I am concerned about. Using the compilation database arguments here may lead to compilation errors that are entirely unrelated to the builtin header path. I really want to make verify the builtin header path here and not any other compilation arguments.

@tobig

tobig Feb 1, 2013

Collaborator

This is the part I am concerned about. Using the compilation database arguments here may lead to compilation errors that are entirely unrelated to the builtin header path. I really want to make verify the builtin header path here and not any other compilation arguments.

+ builtinHeaderPath = getBuiltinHeaderPath(library_path, params["args"])
if not builtinHeaderPath and printWarnings:
print "WARNING: libclang can not find the builtin includes."
@@ -74,11 +88,6 @@ def initClangComplete(clang_complete_flags, clang_compilation_database, \
translationUnits = dict()
global complete_flags
complete_flags = int(clang_complete_flags)
- global compilation_database
- if clang_compilation_database != '':
- compilation_database = CompilationDatabase.fromDirectory(clang_compilation_database)
- else:
- compilation_database = None
global libclangLock
libclangLock = threading.Lock()
return 1

4 comments on commit 478611d

@tobig

This comment has been minimized.

Show comment Hide comment
@tobig

tobig Jan 31, 2013

Collaborator

When checking for the builtin clang includes there seems only a single error possible. That the builtin includes could not be found. Do you see warnings or other error messages.

I would personally prefer if we could add a new configure flag -g:clang_builtin_include_paths, which could be used to just set the builtin include path. This would allow us to be even more sure the error message we give is because of the incorrect builtin include path.

Also, why do you move around the compilation db stuff?

Collaborator

tobig replied Jan 31, 2013

When checking for the builtin clang includes there seems only a single error possible. That the builtin includes could not be found. Do you see warnings or other error messages.

I would personally prefer if we could add a new configure flag -g:clang_builtin_include_paths, which could be used to just set the builtin include path. This would allow us to be even more sure the error message we give is because of the incorrect builtin include path.

Also, why do you move around the compilation db stuff?

@Rip-Rip

This comment has been minimized.

Show comment Hide comment
@Rip-Rip

Rip-Rip Feb 1, 2013

Owner

I was seeing warning due to "unused argument -code-complete-macros", and as such, the diagnostics list was always non empty. Thinking of that, I should probably only add -code-complete-macros if the libclang is not used.

I moved the compilation db stuff due to getCompileParams() that actually use it.

About g:clang_builtin_include_path, I'm not a big fan of having tons of user options, we already have g:clang_user_options for that type of things. Plus, I feel that getBuiltinHeaderPath() only needs some adjustment before it's working for everybody.

Owner

Rip-Rip replied Feb 1, 2013

I was seeing warning due to "unused argument -code-complete-macros", and as such, the diagnostics list was always non empty. Thinking of that, I should probably only add -code-complete-macros if the libclang is not used.

I moved the compilation db stuff due to getCompileParams() that actually use it.

About g:clang_builtin_include_path, I'm not a big fan of having tons of user options, we already have g:clang_user_options for that type of things. Plus, I feel that getBuiltinHeaderPath() only needs some adjustment before it's working for everybody.

@tobig

This comment has been minimized.

Show comment Hide comment
@tobig

tobig Feb 1, 2013

Collaborator
Collaborator

tobig replied Feb 1, 2013

@Rip-Rip

This comment has been minimized.

Show comment Hide comment
@Rip-Rip

Rip-Rip Feb 3, 2013

Owner

An incorrect flag in g:clang_user_options would lead to no completion available later on. Maybe it's good idea to validate those before attempting any completion? (I agree that since I should only have used g:clang_user_options).

I'll move the compilation db stuff in a separate patch.

Here is what I would like to do: fix getBuiltinHeaderPath() so that #238 can be closed and investigate the need for g:clang_builtin_include if we get more issues. Or directly use g:clang_user_options.

Owner

Rip-Rip replied Feb 3, 2013

An incorrect flag in g:clang_user_options would lead to no completion available later on. Maybe it's good idea to validate those before attempting any completion? (I agree that since I should only have used g:clang_user_options).

I'll move the compilation db stuff in a separate patch.

Here is what I would like to do: fix getBuiltinHeaderPath() so that #238 can be closed and investigate the need for g:clang_builtin_include if we get more issues. Or directly use g:clang_user_options.

Please sign in to comment.