[PATCH] D28943: [clang-format] Remove redundant test in style-on-command-line.cpp

[Antonio Maiorano via cfe-commits]

On Fri, 20 Jan 2017 at 11:26 Eric Liu via Phabricator <
reviews at reviews.llvm.org> wrote:

ioeric added a comment.

In https://reviews.llvm.org/D28943#651536, @amaiorano wrote:

> In https://reviews.llvm.org/D28943#651489, @ioeric wrote:
>
> > In https://reviews.llvm.org/D28943#651488, @amaiorano wrote:
> >
> > > In https://reviews.llvm.org/D28943#651470, @ioeric wrote:
> > >
> > > > @amaiorano: The test itself is correct. It's just that this test
failed in our internal test. We could've fixed it internally, but the fix
would be ugly. Since the intended behavior is already covered in the case
above it, and it's really just checking the default fallback style is LLVM,
which is not related to the original change, I think it makes sense to get
rid of the case. Hope you don't mind :)
> > >
> > >
> > > Of course I don't mind :) Why did it fail your internal tests, btw?
Just curious. Was it something I could've detected myself?
> >
> >
> > Probably not... it's just that our default fallback style is "Google"
instead of "LLVM".
>
>
> (I replied the following by email, but I'm not sure where it went...
posting it here instead)
>
> You mean you build a modified version of clang-format where Style is
initialized to getGoogleStyle()?
>
> I had wondered whether adding a "defaultStyle" argument might be useful,
specifically in the case where you want to pass in yaml that simply tweaks
the default style, but I figured it's not much harder to pass in
"BasedOnStyle" in the yaml.


No.. we set both default style and default fallback style (tool options) to
"google" in our build.


How do you do that? I'm trying to understand how running that specific test
failed for you guys. What I'm getting is that my test would not explicitly
set fallback-style and assume it would use LLVM, but you're saying on your
end, there's a way to set the fallback-style otherwise (you wrote "tools
options", but I'm not sure what that means). Sorry for being pedantic :)


I don't see why "defaultStyle" would be useful. I think having the existing
"style" and "fallback-style" options is sufficient (and already a bit
confusing).


I agree completely.




Repository:
  rL LLVM

https://reviews.llvm.org/D28943
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170120/9ec9ecc1/attachment.html>