YouCompleteMe is a vim plugin to allow semantic auto completion (and other fetures) on c/c++ code. It was originally built for chromium and it requires special configurations[1] depending on the build system. I have been using it for a while now, and it might be handy for others webkitgtk developers. [1] https://github.com/scheib/chromium/blob/master/tools/vim/chromium.ycm_extra_conf.py
Created attachment 208411 [details] Patch
Created attachment 219016 [details] Iterated version of Danilo's patch I've worked on Danilo's patch a bit to make it conform to our style guidelines. YCM is great!
I have just a couple of nits in this informal review. Apart from those, the script is great and YCM is a really welcome addition to Vim (I did not know about it before today :D) > Tools/gtk/ycm_extra_conf.py:30 > +sys.path = [__tools_directory] + sys.path It would be good to use os.path.abspath() after determining __tools_directory, just in case. > Tools/gtk/ycm_extra_conf.py:113 > +def FlagsForFile(filename): According to the YouCompleteMe documentation, FlagsForFile may be passed keyword arguments, so this should be: def FlagsForFile(filename, **kwargs) Even if the keyword arguments are unused for the moment, it is good to include them in the function signature just in case users have set an option in their vimrc to send extra arguments to YCM.
Comment on attachment 219016 [details] Iterated version of Danilo's patch View in context: https://bugs.webkit.org/attachment.cgi?id=219016&action=review LGTM, with a small refactoring proposal for the first function > Tools/gtk/ycm_extra_conf.py:47 > + for flag in FLAGS_PRECEDING_PATHS: This for block could be replaced by if argument in FLAGS_PRECEDING_PATHS, you could always ensure argument is either a flag or a path to have a single code path here, it will be much more readable. > Tools/gtk/ycm_extra_conf.py:54 > + # Some argument contain the flag and the path together. For these Missing last bit of the comment? > Tools/gtk/ycm_extra_conf.py:101 > + print "==== Error during reading %s file", trace_file_path s/during/while/?
(In reply to comment #4) > (From update of attachment 219016 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=219016&action=review > > LGTM, with a small refactoring proposal for the first function > > > Tools/gtk/ycm_extra_conf.py:47 > > + for flag in FLAGS_PRECEDING_PATHS: > > This for block could be replaced by if argument in FLAGS_PRECEDING_PATHS, you could always ensure argument is either a flag or a path to have a single code path here, it will be much more readable. The arguments may be of the form -I /path or --argument=path. In the former case, we have to iterate the list of arguments to do a bunch of string searches. I did consider breaking out the argument in FLAGS_PRECEDING_PATHS case like this: for argument in arguments: if make_next_absolute: ... elif argument in FLAGS_PRECEDING_PATHS: make_next_absolute = True else: for flag in FLAGS_PRECEDING_PATHS: if argument.startswith(flag): ... I went with the current approach because it was a little bit more efficient. I like both approaches though, so I can use whichever you prefer. :) > > > Tools/gtk/ycm_extra_conf.py:54 > > + # Some argument contain the flag and the path together. For these > > Missing last bit of the comment? Yes! Thanks. > > > Tools/gtk/ycm_extra_conf.py:101 > > + print "==== Error during reading %s file", trace_file_path > > s/during/while/? Thanks. I missed this.
(In reply to comment #3) > I have just a couple of nits in this informal review. Apart from those, the > script is great and YCM is a really welcome addition to Vim (I did not know > about it before today :D) Thanks for the review! > > Tools/gtk/ycm_extra_conf.py:30 > > +sys.path = [__tools_directory] + sys.path > > It would be good to use os.path.abspath() after determining > __tools_directory, just in case. Okay. > > Tools/gtk/ycm_extra_conf.py:113 > > +def FlagsForFile(filename): > > According to the YouCompleteMe documentation, FlagsForFile may be passed > keyword arguments, so this should be: > > def FlagsForFile(filename, **kwargs) > > Even if the keyword arguments are unused for the moment, it is good to > include them in the function signature just in case users have set an > option in their vimrc to send extra arguments to YCM. Good idea. I'll make the change.
(In reply to comment #6) > (In reply to comment #3) > > > > Tools/gtk/ycm_extra_conf.py:30 > > > +sys.path = [__tools_directory] + sys.path > > > > It would be good to use os.path.abspath() after determining > > __tools_directory, just in case. > > Okay. Also, I forgot to mention that this looks like a more Pythonic way of modifying an array (and it avoids creating temporary values): sys.path.insert(0, __tools_directory)
Committed r160531: <http://trac.webkit.org/changeset/160531>