[PATCH] D50147: clang-format: support external styles

Thu May 23 00:40:00 PDT 2019

Typz added a comment.
Herald added a project: clang.

In D50147#1310146 <https://reviews.llvm.org/D50147#1310146>, @sammccall wrote:

> In D50147#1309880 <https://reviews.llvm.org/D50147#1309880>, @Typz wrote:
>
> > ping?
>
>
> Sorry for losing track of this.
>
> I think `-style=<filename>` is a logical extension of the current options. I'm less sure of supporting it in BasedOnStyle, but happy to go either way.

To keep traceability and ensure consistency, storing a clang-format file in each repository is still required. So BasedOnStyle is actually the feature we need, and from our POV the -style=<filename> extension is a side effect :-)

> Referring to styles by names in installed style directories seems like a relatively marginal feature that would need a more careful design, e.g.:
> 
> - respecting platform conventions

I knew this one was coming: the patch is clearly not complete on this aspect, but I pushed it already to get an early feedback on the generic goal/approach.
This definitely needs some fixing, and to be honest I was hoping there was already something in LLVM code base on this topic (e.g. list of standard path, access to base installation path...) : I tried to look. but did not find anything yet. Any advice?

> - supporting build-time configuration

I thought of injecting the platform-specific path list through the build sytem as well. And this would allow overriding it with any other path list as appropriate.

> - understanding how distro packaging is going to work

I don't understand what you mean exactly. With build-time configuration, the package can be customized  to look in the appropriate places for the specific distribution.

Can you please clarify the issues you see?

> - thinking about addressing the resulting fragmentation as .clang-format files are shared but the referenced files are not Within a tightly controlled organization, these things are less of an issue. We've had luck simply making local changes to support different styles for these scenarios, though that's not ideal.

Our organization is not so controlled, but we want to ease the deployment and maintenance of the styles, hence this approach.
(by the way, ideally I would like to eventually further extend this patch to support retrieval of external styles through url : e.g. to get style from a central server, through http, git....)

> One possibility to reduce the scope here: search for unknown names on under `$CLANG_FORMAT_STYLES` if the variable is set. That way it can be set by administrators within an org if appropriate, but clang-format doesn't have to have opinions about the policy here, and all binaries still behave the same.

I don't think having no search path by default (if the env var does not exist or is empty) is so nice, but I can definitely add such an environment variable override.

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D50147/new/

https://reviews.llvm.org/D50147