clang_complete icon indicating copy to clipboard operation
clang_complete copied to clipboard

.clang_complete - allow inheritance (feature request)

Open boazsegev opened this issue 8 years ago • 15 comments

Hi Xavier,

First, let me thank you for a wonderful plugin.

This feature request was originally posted for the Atom (IDE) package, but since the .clang_complete are your creation and I would love it if they kept persistent behavior across implementations, I'm hoping you'd consider implementing this feature.

I think it would be really great if it were possible that the .clang_complete file could inherit (optionally) from .clang_complete files in parent folders.

Allow me to explain.

The problem

In my user folder, I have a .clang_complete file with all my system's unique configuration options. This allows me to easily access library function for libraries I have installed and wish to work with.

However, some of my projects also need a .clang_complete of their own, since they have special requirements that are listed in their makefile but aren't common to all projects.

Worst, this project custom .clang_complete file is often shared in our git repository (which may have been a mistake), so I can't put my specific system details in there.

The solution

To resolve this, I wish there was a directive I could add to the .clang_complete, telling it it should inherit any options set in a parent folder. i.e., if I could write in the project's .clang_complete:

# #special rules
# ...

# inheritance flag
continue

This way inheritance could propagate all the way to the root of my user profile, or even the root of my machine, if every .clang_complete file along the way had that inheritance flag continue.

Maybe I'm an idiot and this feature already exists, but if it doesn't, it would be great to have :-)

Thank you for your time and have an amazing day, Bo.

boazsegev avatar Jun 30 '16 06:06 boazsegev

As far a I can tell, no inheritance is supported. This function

https://github.com/Rip-Rip/clang_complete/blob/master/plugin/clang_complete.vim#L303

seems to be responsible for parsing .clang_complete file, I imagine you could extend it to support inheritance, I doubt there would be any objections.

graywolf avatar Jun 30 '16 08:06 graywolf

@graywolf is right there is no way to do it at the moment, but it's easy to implement. @boazsegev take a look at config-inheritance branch. I used clang_complete_continue keyword, but better name is welcome.

xaizek avatar Jun 30 '16 12:06 xaizek

Hi @xaizek ,

Thank for putting this together.

I've been deep into maintaining a different project but I hope to have time to look into this today 👍

boazsegev avatar Jul 01 '16 17:07 boazsegev

Hi @xaizek ,

Thanks for the feature, but I have to admit I bumped into some issues when trying to use it.

Because the clang_complete_continue isn't recognized by the main branch, anyone not using the config-inheritance branch was left with an error in the clang_complete, so the plug-in stopped working.

This makes having shared project configurations on top of custom system configurations difficult to achieve for more then a single machine.

I thought maybe if the keyword was hidden in a comment, then it wouldn't break main branch users... but it doesn't seem the clang_complete file has any comments...

So I thought the keyword could hide as a legitimate compiler option, maybe something like:

  -D_clang_complete_continue_

What do you think?

boazsegev avatar Jul 02 '16 09:07 boazsegev

Hi,

Didn't think about such issue, thanks for pointing it out. Your suggestion makes sense, updated the branch.

xaizek avatar Jul 02 '16 09:07 xaizek

that doesn't seem like very clean solution. not that I have a better one

graywolf avatar Jul 04 '16 20:07 graywolf

I agree, a better one would be to use another file for this, but two plugin-specific files nearby is too much.

xaizek avatar Jul 04 '16 20:07 xaizek

On , xaizek wrote:

I agree, a better one would be to use another file for this, but two plugin-specific files nearby is too much.

I just wonder if it would not be better to finish the feature branch, merge it to master and do backward incompatible release.

Since this is just vim plugin, updating it should be trivial.

Yes, this isn't perfect solution either, but in my eyes has way lower technical debt (which someone in my line of work appreciates greatly).

W.

There are only two hard things in Computer Science: cache invalidation, naming things and off-by-one errors.

graywolf avatar Jul 04 '16 20:07 graywolf

I don't think it's worth it. There aren't many things worse than backward incompatible changes in my opinion. And in this particular case source of error won't be immediately obvious.

By the way, is -- used by C++ compilers at all (I don't see it being mentioned in man gcc except for perf)? It could serve as a separator between compiler options and clang_complete options.

xaizek avatar Jul 04 '16 21:07 xaizek

On , xaizek wrote:

I don't think it's worth it. There aren't many things worse than backward incompatible changes in my opinion. And in this particular case source of error won't be immediately obvious.

Well it's your call :)

By the way, is -- used by C++ compilers at all (I don't see it being mentioned in man gcc except for perf)? It could serve as a separator between compiler options and clang_complete options.

$ clang --help | grep -e '--'
--analyze               Run the static analyzer
--cuda-device-only      Do device-side CUDA compilation only
--cuda-host-only        Do host-side CUDA compilation only
--cuda-path=<value>     CUDA installation path
--gcc-toolchain=<value> Use the gcc toolchain at the given directory
--migrate               Run the migrator
--no-system-header-prefix=<prefix>
--system-header-prefix=<prefix>
--target=<value>        Generate code for the given target
--verify-debug-info     Verify the binary representation of debug output

$ gcc --help | grep -e '--'
--help                   Display this information.
--target-help            Display target specific command line options.
--help={common|optimizers|params|target|warnings|[^]{joined|separate|undocumented}}[,...].
(Use '-v --help' to display command line options of sub-processes).
--version                Display compiler version information.
--sysroot=<directory>    Use <directory> as the root directory for headers
Options starting with -g, -f, -m, -O, -W, or --param are automatically

So there seems to be few of them. Never used any of them thought.

W.

There are only two hard things in Computer Science: cache invalidation, naming things and off-by-one errors.

graywolf avatar Jul 04 '16 21:07 graywolf

👍 for making this part of the master branch.

As to backwards compatibility:

A backwards compatible solution must conform to the following property: Anything placed in the .clang_complete plugin file must be a valid compiler option.

This is because clang will complain when any invalid option is provided (i.e. --invalid-option). This, in turn, will break clang_complete (or it does on my computer). Since every line in the plugin file is assumed to be a clang option (even comments aren't permitted), every line must be a valid clang option.

This brings us to two possible backwards compatible solutions.

  1. Use a compiler directive to pass on information to clang_complete. This was the source for my initial proposition, using the -D directive which allows for (almost) any input.
  2. Use a separate plugin file for mid-level directives (i.e. .clang_partial) or clang specific directive (i.e. .clang.conf), which I think as sub-optimal.

My personal opinion:

I think a non backwards compatible solution will work well and will open the door for future backwards compatible feature additions.

The reason is simple:

Older .clang_complete files will work as is - so older projects will not be affected.

At the same time, newer projects which include non backwards compatible directives, will enforce developers to upgrade the vim plugin when working on these projects - which I think is a good thing, as we will get a better collaborative experience as a team working a project.

I do believe that limiting non backwards compatible upgrades is important.

So I would suggest using a prefix for clang_complete directives.

For example, if we use the . prefix for non-critical clang_complete directives and ! for critical directives, we could provide new features with out breaking older versions unless a project requires support for a specific directive.

In out scenario it would allow us to use our keyword .continue to quietly fail if unsupported or !continue to state that this project requires this feature (i.e. to minimize name collision).

...

Just my 2¢

boazsegev avatar Jul 04 '16 22:07 boazsegev

If @xaizek insists on backward compatible, I like the solution with .clang_partial. In root of the project one .clang_complete and in subfolders .clang_partials which automatically extend it (ofc .clang_complete can be in subfolder too and in that case parents are ignored).

But I still think that breaking it in order to improve .clang_complete format would be better.

graywolf avatar Jul 04 '16 22:07 graywolf

Regarding --, I meant to use it as a separator like in

gcc -o out file.c -- anything else

It was past midnight and I somehow thought that if -- is not used, it works as a comment and can help here :-) Which obviously isn't the case.

I was considering renaming the file to something like .clang_partial, but that seems too much for encoding single option. . prefix can conflict with relative paths, I guess some prefix like clang_complete: would do a better job given that in most cases there will be only compiler flags and anything else should standout.

By the way, are we referring to this change as breaking backward compatibility correctly? Wiki says:

a product or technology is backward compatible or downward compatible
if it can function properly given input generated by, or meant for, an older
product or technology

We're discussing forward compatibility in fact:

Forward compatibility is a design characteristic that allows a system to
gracefully accept input intended for a later version of itself.

This kind of changes is fine with me :-) Just need to settle on syntax.

xaizek avatar Jul 05 '16 07:07 xaizek

I would go with following:

!clang_complete: inherit

I think we can be pretty sure no compiler option ever would start with !clang_complete:. And this syntax would allow more additions in similar spirit in the future.

If there is consensus I'll put together pull request (probably based on https://github.com/Rip-Rip/clang_complete/commit/fbe8a4a206d82114c9406304ecd2ae8cf28a3bf1 ).

graywolf avatar Sep 12 '16 18:09 graywolf

!clang_complete: inherit

Nice, I like the syntax.

xaizek avatar Sep 12 '16 19:09 xaizek