Page MenuHomePhabricator

clang-format: Space and triple angle braces
ClosedPublic

Authored by jpienaar on Dec 29 2014, 2:45 PM.

Details

Reviewers
djasper
Summary

clang-format changes "<<<" to "<< <" which is problematic for CUDA code. Change the behavior of clang-format to operate correctly with CUDA kernel launches without changing language options.

  • No space required between "<<" and "<" in "<<<".
  • No space required between identifier and "<<<". This avoids changing "f<<<" to "f <<<"
  • No space required between ">" and "<<<". Avoids changing "f<t><<<" to "f<t> <<<".
  • No space required between ">>>" and "("

Such that "f<param><<<1, 1>>>();" remains unchanged instead of being changed to "f<param> << <1, 1>>> ();"

Diff Detail

Event Timeline

jpienaar updated this revision to Diff 17678.Dec 29 2014, 2:45 PM
jpienaar retitled this revision from to clang-format: Space and triple angle braces.
jpienaar updated this object.
jpienaar edited the test plan for this revision. (Show Details)
jpienaar added a reviewer: djasper.
jpienaar added a subscriber: Unknown Object (MLST).
djasper edited edge metadata.Dec 30 2014, 6:43 AM

We don't generally want to use the spacing in the input to matter for the output. Is this really important? What would be the downside of using language options?

Also, just controlling the space might not be sufficient as clang-format also might go ahead break between the tokens. Generally, it might be better to actually merge the tokens (as is done for the === operation in JavaScript), as these should be considered to be a single token. Alternatively, you need to at least control CanBreakBefore for these and add tests for BreakBeforeBinaryOperators set to All and None.

If CUDA language option is enabled then
A<A<A<int>>> a;
gets changed to
A < A < A < int >>> a;
as >>> at the end is no longer 3 tokens but 1. I could modify the
parseAngle (I believe) function to address this case though. The majority
of uses would be for regular C++, so enabling CUDA language option by
default would seem to benefit me but possibly causing others headaches, so
I went for a route where the opening triple angle bracket is retained as is
in as direct a manner as possible while affecting as few other code paths
as possible.

The importance is in the CUDA code generation side. foo<< <1,1>>>() would
not be recognized as a CUDAKernelCallExpr. So an input program that
compiled would not compile post formatting.

Would it be OK to merge the tokens in the following case?
friend std::ostream& operator << <T> (std::ostream&, const Self&);
That was pretty much the only other usage for three open angle brackets I
saw. Currently I think it would be merged, so that would not change but I
want to double check. If so, then I'll follow your suggestion and add those
tests thanks.

jpienaar updated this revision to Diff 17724.Dec 30 2014, 10:45 AM
jpienaar edited edge metadata.

Version setting CUDA language option for comparison based on discussion earlier today.

The parseAngle function updated to treat >>> as 3 tokens. These are without the binary operator split tests as now both "<<<" and ">>>" are tokens. As the AST is not constructed, setting the CUDA language option should not have other side-effects here other than to recognize <<< and >>> as 1 token each.

djasper added inline comments.Jan 12 2015, 2:01 AM
lib/Format/TokenAnnotator.cpp
68

I really don't like such static variables. Also, now we have two such mechanisms (GreaterStashed and Count). Seems like the same mechanisms should be used.

69

In LLVM style, we don't allow one-liner ifs. Use clang-format ..

71–73

Do these lines actually change the behavior? Seems like the entire wrong place to do this.

1809

So, if we just set TT_TemplateOpener/Closer on these, is this really required?

unittests/Format/FormatTest.cpp
9616

nit: TripleAngleBrackets (one "p" and I don't think that these are "braces").

jpienaar updated this revision to Diff 18057.Jan 12 2015, 3:50 PM

Removed the static variable and perform mergin in FormatTokenLexer. Also ran from clang-format -style llvm which changed some spacing.

The web ui is a bit confusing when I try to reply to comments on previous revision. So I'll reply here. Most of the previous comments probably don't apply since your pointer to GreaterStashed made me redo that part (thanks!). All the rules (except the last one about space between >>> and ( ) do effect the output, and the one about >>> and ( is probably fine. I didn't want to always merge greater, greater, greater into greatergreatergreater as then the template parsing logic needs to be modified, but if there is a lesslessless in the same instruction then that's a reasonably good signal that it is a kernel launch.

jpienaar updated this revision to Diff 19880.Feb 13 2015, 4:00 AM

Redone following a simpler approach of splitting up << into < and <, and remerging only if not <<<. The formatting of kernel launch parameters then follows that of template instantiation, The previous approach could also have resulted in unmatched braces.

Could you generally upload diffs with the full file as context? That makes review here much easier.

lib/Format/Format.cpp
665

Wouldn't we want to fall back to merging in this case?

I think if the user formats the incomplete line "llvm::outs() <<", we should not rip apart the <<. Add a test for this.

674

Make this:

// Only merge if there currently is no whitespace between the two "<".
677

Indentation... Maybe use clang-format ;-).

679

I think this comment doesn't add information. Remove.

lib/Format/TokenAnnotator.cpp
2043

A few lines higher up, there is at test:

if (Left.is(tok::greater) && Right.is(tok::greater) &&
    Left.isNot(TT_TemplateCloser))
  return false;

Seems like we should combine those. Maybe simply remove the "Left.isNot(TT_TemplateCloser)"?

2044

Indentation.

jpienaar updated this revision to Diff 20127.Feb 17 2015, 5:09 PM

Made the changes recommended. clang-format changed unrelated things in the file.

I think that this is basically good to go. Do you have commit access?

lib/Format/Format.cpp
665

Use early exit.

681

Maybe just use "auto" here.

jpienaar updated this revision to Diff 20160.Feb 18 2015, 3:52 AM

Done. I've simplified around line 665 a bit too.

I have commit access and will commit in a couple of hours. Thanks.

djasper accepted this revision.Apr 16 2015, 1:44 AM
djasper edited edge metadata.
This revision is now accepted and ready to land.Apr 16 2015, 1:44 AM
djasper closed this revision.Apr 16 2015, 1:44 AM