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

Thu Jun 6 16:18:32 PDT 2019

sammccall added a comment.

One thing that's unclear to me is whether your aim is to

1. solve a concrete problem for your organization
2. solve a family of problems for similar organizations
3. add a new way of configuring styles for many types of users/projects

If it's 1) I think this is very reasonable and we should focus on finding the simplest mechanism that will work. Is it possible to always set an environment variable, or a build-time parameter, or install the style file in a well-known absolute path (is there really a mix of win/unix users?)
If it's 2), I think what's missing is evidence that other orgs have similar problems and requirements.
Option 3) is interesting, but much more ambitious and needs to start with a design doc that discusses the use cases, alternatives, and implications. Certainly anything involving fetching styles via git/http falls into this bucket, I think.

@klimek in case I'm way off base here, or there have been past discussions that shed light on this.

With that in mind, I'd be very happy to approve the build time config and/or an env variable, as long as they're off by default. It's easy to turn them on later, but not easy to turn them off.
If they're going to be on by default, I think we need a strong reason.

In D50147#1513166 <https://reviews.llvm.org/D50147#1513166>, @Typz wrote:

> In D50147#1310146 <https://reviews.llvm.org/D50147#1310146>, @sammccall wrote:
>
> > - 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?

There's lots of code in Driver that manipulates search paths in platform-specific ways :-) Most of my experience with that urges me to avoid it if at all possible, and otherwise keep it very simple.

In D50147#1513166 <https://reviews.llvm.org/D50147#1513166>, @Typz wrote:

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

There's a mechanism, but how is it to be used? Will/should projects with a style guide provide style packages for distros? Or should these be part of the "official" clang-format package? 
If separate packages exist, how much is it going to confuse users that clang-format will silently format the same project with a `.clang-format` file different ways depending on what's installed?

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

================
Comment at: lib/Format/Format.cpp:1080
+                     llvm::vfs::FileSystem *FS = nullptr) {
+  if (!FS) {
+    FS = llvm::vfs::getRealFileSystem().get();
----------------
This fallback silently changes callers of getPredefinedStyle() with no FS to read from the real filesystem (if the name doesn't match anything).
This seems bad for embedders, and it's not obvious what their workaround is. I'd suggest we either want to change the signature (e.g. by not having a default param) and force them to choose, or make nullptr mean no FS and document the inconsistency with getStyle().

Repository:
  rC Clang

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

https://reviews.llvm.org/D50147