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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I tend to agree, I'm not keen on the silent searching for the files.. this 
happens too much as it is, with Microsoft releasing VisualStudio with a 
clang-format.exe (v5.0), I suddenly find that binary is in my path, then all of 
a sudden all the  5.0 bugs we fixed since 5.0 reappear.

Being specific seems much better than searching my %PROFILE%


Repository:
  rC Clang

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

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D50147#1751198 , @Typz wrote:

> >> In our case, we actually have more than one "standard" style, a 
> >> combination of various OS (linux, windows, macos), and not a very strong 
> >> control on user computers. So we cannot rely on a specific file or 
> >> variable being setup by an administrator.
> > 
> > In this case my best advice would be in the short term to use .clang-format 
> > files. Longer term, some combination of using well-known styles, 
> > publicising and teaching clang-format about the styles you use, and gaining 
> > the ability to set environment variables would reduce duplication.
>
> what do you mean about "gaining the ability to set environment variables" ?


If you can set a CLANG_FORMAT_STYLE_PATH or so environment variable, then you 
can install your styles there

> - Build option is implemented. This allows turn the feature off if needed, at 
> build time (by specifying empty search path). I would prefer to keep thos
> - Overriding an environment varialbe to change the search path is fine by me. 
> But I would still prefer to have a "working" default, so that it can be used 
> out-of-the-box, with no extra env variable to set

OK, I'm not going to approve turning this feature on by default (specifically: 
searching unknown strings as paths relative to some directory baked into the 
binary). I think it's bad for the ecosystem.
If you want to use absolute paths, or set the path with a build option, 
environment variable, or flag that seems fine to me.

> The goal is indeed that user keep installing clang-format through  LLVM 
> releases or OS distributors; but they would also install the custom styles 
> they need, which would be provided by their organization (not clang/llvm!): 
> e.g. Qt style, or "my-compagny" style.

My concern about the ecosystem is largely about requiring users to know which 
styles they need.


Repository:
  rC Clang

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

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In D50147#1648786 , @sammccall wrote:

> > First and forehand, I have a problem to solve in my organization : we have 
> > many projects, and maintaining the config file in some many repositories is 
> > not practical.
> >  In some case (like, LLVM, google or mozilla), this is not an issue because 
> > the formatting rules are hard-coded in clang-format.
>
> I think this option is available to anyone with a public style guide that 
> covers a reasonably large body of open-source code, and which can be 
> reasonably well supported by clang-format.
>  I'd expect QT, KDE etc should be able to use this mechanism.


unfortunately this is not open-source code, so we cannot modify clang-format :-(
hence the will to have the exact same behavior, but without modifying 
clang-format itself

>> In our case, we actually have more than one "standard" style, a combination 
>> of various OS (linux, windows, macos), and not a very strong control on user 
>> computers. So we cannot rely on a specific file or variable being setup by 
>> an administrator.
> 
> In this case my best advice would be in the short term to use .clang-format 
> files. Longer term, some combination of using well-known styles, publicising 
> and teaching clang-format about the styles you use, and gaining the ability 
> to set environment variables would reduce duplication.

what do you mean about "gaining the ability to set environment variables" ?
in our case, we have around 500 repositories, so it really becomes a problem 
maintaining hundreds of copies of each style and verifying projects do not have 
a "variation" on one of the official style.

>> I think many orgs have the same issue, but some of them have found a 
>> solution by hard-coding their style in clang-format...
> 
> I'd like to see evidence that this is a widespread problem.

indeed I have no number at all.
But I think in many case, people would either:

- switch to a built-in style (which is not bad in itself, but can be 
problematic if there is lot of code already),
- not to use clang-format at all (which does not help with ensuring consistent 
formatting),
- or "fork" clang-format (which would we are currently forced to do, but is 
really not efficient)

>>> 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.
>> 
>> I they are going to be off by default, it means we would still need to patch 
>> clang-format to use it, correct ?
> 
> Sorry, by "off by default" I mean that if the environment variable is not 
> set, there would be no default search directory. Relative paths would be an 
> error.
>  So you could install styles centrally on each machine, and they would work 
> if CLANG_FORMAT_STYLE_PATH was set, otherwise you'd get the fallback style. 
> Would that be workable?



- Build option is implemented. This allows turn the feature off if needed, at 
build time (by specifying empty search path). I would prefer to keep thos
- Overriding an environment varialbe to change the search path is fine by me. 
But I would still prefer to have a "working" default, so that it can be used 
out-of-the-box, with no extra env variable to set

>> In D50147#157 , @sammccall 
>> wrote:
>> 
>>> >> - understanding how distro packaging is going to work
>>>
>>> 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?

The exact same thing happens today with clang-format's integrated styles.

But indeed this is an issue, and handling it would be my next step : once the 
concept of 'external style' is present, I'd like to allow referencing a style 
stored in remote git server or other URL, with clang-format handling the 
retrieval/update/cache.

>> The goal is to actually separate the styles from clang-format : so I don't 
>> see the point to make them part of the official clang-format package.
>>  Usage may be different: the styles may be setup through different packages 
>> (e.g. Qt style in qt-core package), installed manually by user, 
>>  This is surely not perfect, since different packages may indeed provide the 
>> same style : technically this is not an issue (packages must just be marked 
>> as conflicting), but it is indeed the organisation's responsibility to use 
>> different names for the styles...
>>  (to some extent, this may be improved by passing URLs for external styles, 
>> e.g. from git ; but this may 

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

but perhaps I'm really just agreeing to @sammccall  's suggestion of 
"-style=" which kind of feels sufficient to me..


Repository:
  rC Clang

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

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I am not opposed to this idea, I actually think this has some mileage based on 
a use case I've encountered:

our team tends to use windows builds of clang-format from 
(https://llvm.org/builds/) because its easy to distribute the installer, this 
is ok but sometimes these snapshot builds can lag new features by 1-2 months

This can make introducing a new configuration option a problem because it 
requires us to have to get everyone upgraded before we can change the checked 
in .clang-format at the top of the tree.

If everyone isn't upgraded they start getting complaints about unknown 
configurations. Its not 100% obvious if those styles are being ignored, if no 
formatting at all is being done or if when clang-format is embedded inside 
Visual Studio or VIM what exactly is happening.

I see this feature would allow those on the bleeding edge to use an alternate 
.clang-format file (even if that was also in the root /.clang-format-next


Repository:
  rC Clang

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

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a subscriber: MyDeveloperDay.
sammccall added a comment.

Sorry about the long delays in responses.
I don't think the feature, as you want to scope it, belongs in clang-format.
@klimek @MyDeveloperDay may have different opinions.

In D50147#1533892 , @Typz wrote:

> In D50147#157 , @sammccall wrote:
>
> > 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
>
>
> First and forehand, I have a problem to solve in my organization : we have 
> many projects, and maintaining the config file in some many repositories is 
> not practical.
>  In some case (like, LLVM, google or mozilla), this is not an issue because 
> the formatting rules are hard-coded in clang-format.


I think this option is available to anyone with a public style guide that 
covers a reasonably large body of open-source code, and which can be reasonably 
well supported by clang-format.
I'd expect QT, KDE etc should be able to use this mechanism.

> In our case, we actually have more than one "standard" style, a combination 
> of various OS (linux, windows, macos), and not a very strong control on user 
> computers. So we cannot rely on a specific file or variable being setup by an 
> administrator.

In this case my best advice would be in the short term to use .clang-format 
files. Longer term, some combination of using well-known styles, publicising 
and teaching clang-format about the styles you use, and gaining the ability to 
set environment variables would reduce duplication.

> I think many orgs have the same issue, but some of them have found a solution 
> by hard-coding their style in clang-format...

I'd like to see evidence that this is a widespread problem.

>> 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.
> 
> I they are going to be off by default, it means we would still need to patch 
> clang-format to use it, correct ?

Sorry, by "off by default" I mean that if the environment variable is not set, 
there would be no default search directory. Relative paths would be an error.
So you could install styles centrally on each machine, and they would work if 
CLANG_FORMAT_STYLE_PATH was set, otherwise you'd get the fallback style. Would 
that be workable?

> In D50147#157 , @sammccall wrote:
> 
>> >> - understanding how distro packaging is going to work
>>
>> 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?
> 
> 
> The goal is to actually separate the styles from clang-format : so I don't 
> see the point to make them part of the official clang-format package.
>  Usage may be different: the styles may be setup through different packages 
> (e.g. Qt style in qt-core package), installed manually by user, 
>  This is surely not perfect, since different packages may indeed provide the 
> same style : technically this is not an issue (packages must just be marked 
> as conflicting), but it is indeed the organisation's responsibility to use 
> different names for the styles...
>  (to some extent, this may be improved by passing URLs for external styles, 
> e.g. from git ; but this may even be an issue if different mirrors must be 
> used...)

The large majority of our users install clang-format through LLVM releases or 
OS distributors that repackage the same releases. There's no "the organization" 
to defer responsibility to; that's us.


Repository:
  rC Clang

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

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


Repository:
  rC Clang

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

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In D50147#157 , @sammccall wrote:

> 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


First and forehand, I have a problem to solve in my organization : we have many 
projects, and maintaining the config file in some many repositories is not 
practical.
In some case (like, LLVM, google or mozilla), this is not an issue because the 
formatting rules are hard-coded in clang-format.
In other large organisation (like Qt, KDE or many private compagnies), there 
may be coding-rules already in place and it is not practical to store the 
configuration in clang-format.
(This is not so much an issue in small-organisation, with few 
projects/repositories : each project/repo may manage its own clang-format 
config file)

The goal is to support this use-case : organisation-wide styles, without 
patching clang-format.

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

In our case, we actually have more than one "standard" style, a combination of 
various OS (linux, windows, macos), and not a very strong control on user 
computers. So we cannot rely on a specific file or variable being setup by an 
administrator.
Therefore, I think the solution should still be generic enough, and there is no 
reason to restrict it to one use case.

> If it's 2), I think what's missing is evidence that other orgs have similar 
> problems and requirements.

I think many orgs have the same issue, but some of them have found a solution 
by hard-coding their style in clang-format...

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

git/http/... is indeed much more complicated. It would solve the issue of 
"transparent" setup, but introduces many subtleties : how to cache the result 
(to handle offline use or downtime issues), how to handle updates

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

I they are going to be off by default, it means we would still need to patch 
clang-format to use it, correct ?
I would like to avoid this, as it is quite time consuming; and it would 
actually be easier in that case to just add yet another hard-coded style in the 
patched binary...

In D50147#157 , @sammccall wrote:

> >> - understanding how distro packaging is going to work
>
> 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?


The goal is to actually separate the styles from clang-format : so I don't see 
the point to make them part of the official clang-format package.
Usage may be different: the styles may be setup through different packages 
(e.g. Qt style in qt-core package), installed manually by user, 
This is surely not perfect, since different packages may indeed provide the 
same style : technically this is not an issue (packages must just be marked as 
conflicting), but it is indeed the organisation's responsibility to use 
different names for the styles...
(to some extent, this may be improved by passing URLs for external styles, e.g. 
from git ; but this may even be an issue if different mirrors must be used...)


Repository:
  rC Clang

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

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

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 , @Typz wrote:

> In 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 , @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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


Repository:
  rC Clang

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

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 201693.
Typz marked 3 inline comments as done.
Typz added a comment.
Herald added subscribers: ormris, mgorny.

- Rebased
- Adapt styles search path to platform conventions
- Allow customizing search path at compile time
- Allow overriding search path at runtime through CLANG_FORMAT_STYLES_PATH env 
variable


Repository:
  rC Clang

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

https://reviews.llvm.org/D50147

Files:
  include/clang/Format/Format.h
  lib/Format/CMakeLists.txt
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -13185,6 +13185,124 @@
   ASSERT_EQ(*Style1, getGoogleStyle());
 }
 
+namespace {
+class EnvSetter {
+StringRef m_key;
+Optional m_oldValue;
+public:
+EnvSetter(StringRef key, StringRef value = StringRef()) : m_key(key) {
+  if (const char *oldValue = ::getenv(m_key.data()))
+m_oldValue.emplace(oldValue);
+  if (value.empty())
+::unsetenv(m_key.data());
+  else
+::setenv(m_key.data(), value.data(), 1);
+}
+~EnvSetter() {
+  if (m_oldValue)
+::setenv(m_key.data(), m_oldValue.getValue().data(), 1);
+}
+EnvSetter(const EnvSetter &) = delete;
+EnvSetter =(const EnvSetter &) = delete;
+};
+}
+
+TEST(FormatStyle, GetExternalStyle) {
+  // Override HOME environment variable to make tests independant of actual user
+  EnvSetter homeEnv("HOME", "/home");
+
+  // Clear CLANG_FORMAT_STYLES_PATH environment variable
+  EnvSetter stylesPathEnv("CLANG_FORMAT_STYLES_PATH");
+
+  llvm::vfs::InMemoryFileSystem FS;
+
+  // Test 1: format file in /usr/local/share/clang-format/
+  ASSERT_TRUE(
+  FS.addFile("/usr/local/share/clang-format/style1", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  auto Style1 = getStyle("style1", "", "LLVM", "", );
+  ASSERT_TRUE((bool)Style1);
+  ASSERT_EQ(*Style1, getGoogleStyle());
+
+  // Test 2: format file in /usr/share/clang-format/
+  ASSERT_TRUE(
+  FS.addFile("/usr/share/clang-format/style2", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  auto Style2 = getStyle("style2", "", "LLVM", "", );
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getGoogleStyle());
+
+  // Test 3: format file in ~/.local/share/clang-format/
+  ASSERT_TRUE(
+  FS.addFile("/home/.local/share/clang-format/style3", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  auto Style3 = getStyle("style3", "", "LLVM", "", );
+  ASSERT_TRUE((bool)Style3);
+  ASSERT_EQ(*Style3, getGoogleStyle());
+
+  // Test 4: format file in absolute path
+  ASSERT_TRUE(
+  FS.addFile("/clang-format-styles/style4", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  auto Style4 = getStyle("/clang-format-styles/style4", "", "LLVM", "", );
+  ASSERT_TRUE((bool)Style4);
+  ASSERT_EQ(*Style4, getGoogleStyle());
+
+  // Test 5: format file in relative path
+  ASSERT_TRUE(
+  FS.addFile("/clang-format-styles/style5", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  FS.setCurrentWorkingDirectory("/clang-format-styles");
+  auto Style5 = getStyle("style5", "", "LLVM", "", );
+  ASSERT_TRUE((bool)Style5);
+  ASSERT_EQ(*Style5, getGoogleStyle());
+  FS.setCurrentWorkingDirectory("/");
+
+  // Test 6: file does not exist
+  auto Style6 = getStyle("style6", "", "LLVM", "", );
+  ASSERT_FALSE((bool)Style6);
+  llvm::consumeError(Style6.takeError());
+
+  // Test 7: absolute file does not exist
+  auto Style7 = getStyle("/style7", "", "LLVM", "", );
+  ASSERT_FALSE((bool)Style7);
+  llvm::consumeError(Style7.takeError());
+
+  // Test 8: file is not a format style
+  ASSERT_TRUE(
+  FS.addFile("/usr/local/share/clang-format/nostyle", 0,
+ llvm::MemoryBuffer::getMemBuffer("This is not a style...")));
+  FS.setCurrentWorkingDirectory("/home/clang-format-styles");
+  auto Style8 = getStyle("nostyle", "", "LLVM", "", );
+  ASSERT_FALSE((bool)Style8);
+  llvm::consumeError(Style8.takeError());
+
+  {
+// Override styles path
+EnvSetter customStylesPathEnv("CLANG_FORMAT_STYLES_PATH", "/customPath1:/customPath2");
+
+// Test 8: file in custom styles path
+ASSERT_TRUE(
+FS.addFile("/customPath1/style8_1", 0,
+   llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+auto Style8_1 = getStyle("style8_1", "", "LLVM", "", );
+ASSERT_TRUE((bool)Style8_1);
+ASSERT_EQ(*Style8_1, getGoogleStyle());
+
+ASSERT_TRUE(
+FS.addFile("/customPath2/style8_2", 0,
+   llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Mozilla")));
+auto Style8_2 = getStyle("style8_2", "", "LLVM", "", );
+ASSERT_TRUE((bool)Style8_2);
+

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

[Francois Ferrand via Phabricator via cfe-commits]

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

In D50147#1310146 , @sammccall wrote:

> In D50147#1309880 , @Typz wrote:
>
> > ping?
>
>
> Sorry for losing track of this.
>
> I think `-style=` 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= 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D50147#1309880 , @Typz wrote:

> ping?


Sorry for losing track of this.

I think `-style=` is a logical extension of the current options. I'm 
less sure of supporting it in BasedOnStyle, but happy to go either way.

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
- supporting build-time configuration
- understanding how distro packaging is going to work
- 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.
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.




Comment at: lib/Basic/VirtualFileSystem.cpp:288
 SmallVectorImpl ) const {
-  return llvm::sys::fs::real_path(Path, Output);
+  return llvm::sys::fs::real_path(Path, Output, true);
 }

(this change isn't a good idea - if you want to expand tilde call 
fs::expand_tilde - it's not related to VFS)


Repository:
  rC Clang

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

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

ping?


Repository:
  rC Clang

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

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D50147#1272742, @sammccall wrote:

> Being able to discover the right style from the filesystem is powerful, and 
> if I was going to use this flag, I'd consider symlinking the style-file to 
> `subproject/.clang_format` instead. That way the setting is persistent and 
> available to all tools, rather than us needing to get it right on each 
> invocation.


I did not think of this solution. However:

- I am not sure how it works when the .clang-format link is stored in SCM: not 
sure git will allow storing a link which points outside of the repo... But it 
may be worth trying, it could be an alternative
- Making a symlink will hardcode the full path of the style, which may not suit 
different users: maybe in some cases the styles could be installed per-user (as 
users do not have root permissions), which they would be installed system-wide 
(in containers used for CI builds, where we may not know which user will 
actually run the build)
- Making a symlink will definitely not be portable to different OS : esp. on 
Windows, the path would probably not be the same
- I am not sure a symlink can point to a "~"-relative path, and automatically 
perform the user HOME path resolution




Comment at: lib/Format/Format.cpp:938
+  const llvm::SmallVector paths = {
+{"/usr/local/share/clang-format/", Name},
+{"~/.local/share/clang-format/", Name},

sammccall wrote:
> I'm not sure these prefixes are a good idea - can you explain what the 
> purpose is, and why the simpler model of just using the path doesn't work?
> 
> Certainly this needs some more thought about how it would work on windows 
> etc. I'd suggest limiting this patch to filenames.
the goal is actually to store these "base" styles in some global folder, so 
that multiple projects can reference it (through their own .clang-format file), 
without having to make any copy.

The idea is that the project is under SCM, and simply includes a reference to 
the style: in its own .clang-format, which would ideally only contain a single 
//basedOn// tag. The styles are defined globally, then each project only 
indicates which styles in uses.

But indeed, the 'HOME' folder on windows is probably not this one...


Repository:
  rC Clang

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In https://reviews.llvm.org/D50147#1279056, @Typz wrote:

> In https://reviews.llvm.org/D50147#1272742, @sammccall wrote:
>
> > The idea here does seem to be a natural extension of -style, at least for 
> > the case when the arg is a filename directly. I'm not opposed, happy to 
> > review this.
> >
> > I do want to probe the use case a bit though: we have found that 
> > configuring via -style= doesn't scale that well to lots of codebases and 
> > tools. e.g. if someone's running clang-tidy or clangd via some driver, are 
> > they going to have a way to plumb through the right -style=?
> >
> > Being able to discover the right style from the filesystem is powerful, and 
> > if I was going to use this flag, I'd consider symlinking the style-file to 
> > `subproject/.clang_format` instead. That way the setting is persistent and 
> > available to all tools, rather than us needing to get it right on each 
> > invocation.
>
>
> I totally agree: using '-style' does not scale well. However this patch works 
> also when the style is used in the 'basedOn' tag. This is actually the way we 
> expect to use this: store a small .clang_format file in each repo, which 
> simply references a clang-format file which must be installed (manually, for 
> now) on each user's computer.


`BasedOn: "/path/to/some/file"` would certainly work; but why not just use a 
symlink?


Repository:
  rC Clang

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D50147#1272742, @sammccall wrote:

> The idea here does seem to be a natural extension of -style, at least for the 
> case when the arg is a filename directly. I'm not opposed, happy to review 
> this.
>
> I do want to probe the use case a bit though: we have found that configuring 
> via -style= doesn't scale that well to lots of codebases and tools. e.g. if 
> someone's running clang-tidy or clangd via some driver, are they going to 
> have a way to plumb through the right -style=?
>
> Being able to discover the right style from the filesystem is powerful, and 
> if I was going to use this flag, I'd consider symlinking the style-file to 
> `subproject/.clang_format` instead. That way the setting is persistent and 
> available to all tools, rather than us needing to get it right on each 
> invocation.


I totally agree: using '-style' does not scale well. However this patch works 
also when the style is used in the 'basedOn' tag. This is actually the way we 
expect to use this: store a small .clang_format file in each repo, which simply 
references a clang-format file which must be installed (manually, for now) on 
each user's computer.


Repository:
  rC Clang

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

The idea here does seem to be a natural extension of -style, at least for the 
case when the arg is a filename directly. I'm not opposed, happy to review this.

I do want to probe the use case a bit though: we have found that configuring 
via -style= doesn't scale that well to lots of codebases and tools. e.g. if 
someone's running clang-tidy or clangd via some driver, are they going to have 
a way to plumb through the right -style=?

Being able to discover the right style from the filesystem is powerful, and if 
I was going to use this flag, I'd consider symlinking the style-file to 
`subproject/.clang_format` instead. That way the setting is persistent and 
available to all tools, rather than us needing to get it right on each 
invocation.




Comment at: lib/Format/Format.cpp:938
+  const llvm::SmallVector paths = {
+{"/usr/local/share/clang-format/", Name},
+{"~/.local/share/clang-format/", Name},

I'm not sure these prefixes are a good idea - can you explain what the purpose 
is, and why the simpler model of just using the path doesn't work?

Certainly this needs some more thought about how it would work on windows etc. 
I'd suggest limiting this patch to filenames.


Repository:
  rC Clang

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D50147#1244237, @krasimir wrote:

> I don't understand the use-case this patch is realizing. Suppose I have a 
> bunch of projects that have to share a format style. Then I can checkout them 
> under a common root directory and put an appropriate .clang-format file there.


You can do this manually, but this would not be in source-control and not 
"automatic" : you need to remember to copy the file in the root directory, and 
be sure to copy the correct one.
Also, this manual copy approach does not support updates very well.

We typically use large workspaces with many repositories (using git-repo), 
containing both open-source projects (hopefull with their own `.clang-format`, 
but more often not without any) and in-house projects.
Putting a .clang-format in the root directory would make it the default not 
only for our projects, but also for all OSS projects with no .clang-format, 
where it likely is not correct.
In addition, due to historical or regulatory reasons we unfortunately have 
multiple coding rules for the same language, in different projects : so we 
cannot just put at the root directory.

Allowing to implement "custom" styles outside of clang-format fixes this, by 
allowing to define this style globally; and then simpy reference the style in 
the project.
We can then also version the styles, e.g. create myStyle-1.0 and myStyle-1.1 to 
handle changes and let projects upgrade to the latest version as needed, and 
possibly create links to allow "tracking" the latest revision of a style (e.g. 
myStyle-latest is a symlink which points to myStyle-1.1).


Repository:
  rC Clang

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added a comment.

I don't understand the use-case this patch is realizing. Suppose I have a bunch 
of projects that have to share a format style. Then I can checkout them under a 
common root directory and put an appropriate .clang-format file there.


Repository:
  rC Clang

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

clang-format does indeed support .clang-format, which is great for *isolated* 
projects, and which is not affected by this patch.

This patch addresses the issue of *centralizing* the definition of styles, e.g. 
allowing individual projects to reference externally defined styles. This is 
useful when deploying a coding styles compagny-wide, to avoid copying the 
configuration in each project (and later having problem issues to maintain the 
styles or check if the correct style is indeed used). Another way of seeing it 
is that it allows extending the styles which are hard-coded in clang-format 
(llvm, google, ...)


Repository:
  rC Clang

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Arnaud Coomans via Phabricator via cfe-commits]

acoomans added a comment.

Doesn’t clang-format support project-based //.clang-format// or 
//_clang-format//? How do they play with this diff?


Repository:
  rC Clang

https://reviews.llvm.org/D50147



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Francois Ferrand via Phabricator via cfe-commits]

Typz created this revision.
Typz added reviewers: krasimir, djasper, klimek.
Herald added a subscriber: acoomans.

This patch allows defining external styles, which can be used exactly
like the embedded styles (llvm, google, mozilla...).

These styles are clang-format files installed either systemwide in
/usr/local/share/clang-format, or per-user in ~/.local/share/clang-
format. These can be specified by simply using the name of the file,
and clang-format will search the directories for the style:

  clang-format -style=foo-1.0

The patch also allows loading specifying a file name directly, either
relative or absolute:

  clang-format -style=/home/clang-format-styles/foo-1.0
  clang-format -style=styles/foo-1.0

This works also in `BaseOnStyle` field, which allows defining compagny-
wide (and possibly versionned) clang-format styles, without having to
maintain many copies in each repository: each repository will simply
need to store a short .clang-format, which simply references the
compagny-wide style.

The drawback is that these style need to be installed on each computer,
but this may be automated through an OS package. In any case, the error
cannot be ignored, as the user will be presented with an error message
if he does not have the style.

NOTE: this may be further improved by also allowing URL (http://,
git://...) in this field, which would allow clang-format to automatically
download the missing styles.


Repository:
  rC Clang

https://reviews.llvm.org/D50147

Files:
  include/clang/Format/Format.h
  lib/Basic/VirtualFileSystem.cpp
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12113,6 +12113,61 @@
   ASSERT_EQ(*Style1, getGoogleStyle());
 }
 
+TEST(FormatStyle, GetExternalStyle) {
+  vfs::InMemoryFileSystem FS;
+  // Test 1: format file in /usr/local/share/clang-format/
+  ASSERT_TRUE(
+  FS.addFile("/usr/local/share/clang-format/style1", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  auto Style1 = getStyle("style1", "", "LLVM", "", );
+  ASSERT_TRUE((bool)Style1);
+  ASSERT_EQ(*Style1, getGoogleStyle());
+
+  // Test 2: format file in ~/.local/share/clang-format/
+  ASSERT_TRUE(
+  FS.addFile("~/.local/share/clang-format/style2", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  auto Style2 = getStyle("style2", "", "LLVM", "", );
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getGoogleStyle());
+
+  // Test 3: format file in absolute path
+  ASSERT_TRUE(
+  FS.addFile("/clang-format-styles/style3", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  auto Style3 = getStyle("/clang-format-styles/style3", "", "LLVM", "", );
+  ASSERT_TRUE((bool)Style3);
+  ASSERT_EQ(*Style3, getGoogleStyle());
+
+  // Test 4: format file in relative path
+  ASSERT_TRUE(
+  FS.addFile("/home/clang-format-styles/style4", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
+  FS.setCurrentWorkingDirectory("/home/clang-format-styles");
+  auto Style4 = getStyle("style4", "", "LLVM", "", );
+  ASSERT_TRUE((bool)Style4);
+  ASSERT_EQ(*Style4, getGoogleStyle());
+
+  // Test 5: file does not exist
+  auto Style5 = getStyle("style5", "", "LLVM", "", );
+  ASSERT_FALSE((bool)Style5);
+  llvm::consumeError(Style5.takeError());
+
+  // Test 6: absolute file does not exist
+  auto Style6 = getStyle("/style6", "", "LLVM", "", );
+  ASSERT_FALSE((bool)Style6);
+  llvm::consumeError(Style6.takeError());
+
+  // Test 7: file is not a format style
+  ASSERT_TRUE(
+  FS.addFile("/usr/local/share/clang-format/nostyle", 0,
+ llvm::MemoryBuffer::getMemBuffer("This is not a style...")));
+  FS.setCurrentWorkingDirectory("/home/clang-format-styles");
+  auto Style7 = getStyle("nostyle", "", "LLVM", "", );
+  ASSERT_FALSE((bool)Style7);
+  llvm::consumeError(Style7.takeError());
+}
+
 TEST(FormatStyle, GetStyleOfFile) {
   vfs::InMemoryFileSystem FS;
   // Test 1: format file in the same directory.
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -928,8 +928,42 @@
   return NoStyle;
 }
 
+// Try loading config file from path, in argument to -style and BasedOnStyle.
+bool loadSystemStyle(StringRef Name, FormatStyle *Style,
+ vfs::FileSystem *FS = nullptr) {
+  if (!FS) {
+FS = vfs::getRealFileSystem().get();
+  }
+  const llvm::SmallVector paths = {
+{"/usr/local/share/clang-format/", Name},
+{"~/.local/share/clang-format/", Name},
+Name
+  };
+  for (Twine Path: paths) {
+llvm::SmallVector RealPath;
+Twine ConfigFile{!FS->getRealPath(Path, RealPath) ? RealPath : Path};
+if (FS->exists(ConfigFile)) {
+  llvm::ErrorOr> Text =