Bug 119618 – Adding a .ycm_extra_conf file for webkitGtk

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Danilo de Paula]

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

[Danilo de Paula]

Created attachment 208411 [details]
Patch

[Martin Robinson]

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!

[Adrian Perez]

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.

[Gustavo Noronha (kov)]

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/?

[Martin Robinson]

(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.

[Martin Robinson]

(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.

[Adrian Perez]

(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)

[Martin Robinson]

Committed r160531: <http://trac.webkit.org/changeset/160531>