Page MenuHomePhabricator

Add examples to clang-format configuration
ClosedPublic

Authored by sylvestre.ledru on Mar 2 2017, 5:54 AM.

Diff Detail

Event Timeline

sylvestre.ledru created this revision.Mar 2 2017, 5:54 AM

If you think it makes sense, I will do that for the rest of the options

klimek added inline comments.Mar 2 2017, 6:38 AM
docs/ClangFormatStyleOptions.rst
218

Do we really align these 3 spaces out?

447

Don't we align the : further to the right?

517

I thought we don't do that for single lines?

731

Missing the space?

sylvestre.ledru marked 3 inline comments as done.
sylvestre.ledru added inline comments.
docs/ClangFormatStyleOptions.rst
218

nope, bad copy and paste it seems. Sorry

447

Yes, sorry, issue during the copy and paste :(

517

Yes and no. I reported this bug:
https://bugs.llvm.org//show_bug.cgi?id=32117
I will update the example to avoid the confusion

With the rst generation

djasper edited edge metadata.Mar 3 2017, 1:53 AM

Hm. I don't actually know whether these examples are useful as is. You only present one setting of the value in most cases. What's interesting is actually how the flag changes the behavior. I mean in most cases, this can be derived from the example, but in those cases, it's also fairly obvious what the flag does. Unfortunately, I also don't have a significantly better idea. Maybe something like https://clangformat.com/ is just better at handling this?

include/clang/Format/Format.h
661

"update" seems like the wrong word. Maybe "affected"?

I see a lot of values in examples. If I started this review, it is because I was lost in all the options and could not find what I was looking for.

If you want, I can update the example to provide results with and without the option.

like

With SpacesInCStyleCastParentheses: true:
x = ( int32 )y
With false:
x = (int32)y

Sure, then go ahead. If these examples would have helped you, that's one datapoint :).

As for presenting the difference in options, that would be useful I think. So if you are up to also doing that, that'd be appreciated.
For bool options it seems easiest to do something like:

true:  x = ( int32 )y;
false: x = (int32)y;

(I'd try to keep this as compact as possible for now as the style page getting much longer also hurts discoverability)

kimgr added a subscriber: kimgr.Mar 3 2017, 2:08 AM

For what it's worth, I also find this useful to be able to see at a glance what the setting does. I know I've tried several different settings in the past before finding the one I was looking for.

For bool options it seems easiest to do something like:
true: x = ( int32 )y;
false: x = (int32)y;

That works for single declarations but for stuff like:

SomeClass::Constructor()
        : a(a)
        , b(b)
        , c(c) {}

I won't be great :/

I agree, just generally we should aim for keeping these short.

The example you gave might actually be reasonable to compare in two columns, i.e.:

true:                                  false:
SomeClass::Constructor()      vs.      SomeClass::Constructor() : a(a),
        : a(a)                                                    b(b),
        , b(b)                                                    c(c) {}
        , c(c) {}

or some such.

I also think that examples for the flags are good. My use case is that while developing/debugging its nice to see a short example for a random flag in the documentation pop-up.

With the changes requested by Daniel. This is much better indeed!

updated => affected
+ align the vs

sylvestre.ledru marked an inline comment as done.Mar 3 2017, 7:57 AM
sylvestre.ledru marked 2 inline comments as done.Mar 6 2017, 6:58 AM

Daniel, it is good for me now.
Once it is accepted, I will work on the rest in a separate commit if that is ok with you;

djasper accepted this revision.Mar 6 2017, 8:12 AM

Looks good. Thank you for doing this!

This revision is now accepted and ready to land.Mar 6 2017, 8:12 AM