This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Recognize Java logical shift assignment operator
ClosedPublic

Authored by bradfier on Apr 4 2017, 2:44 AM.

Details

Summary

At present, clang-format mangles Java containing logical right shift operators ('>>>=' or '>>>'), splitting them in two, resulting in invalid code:

 public class Minimal {
   public void func(String args) {
     int i = 42;
-    i >>>= 1;
+    i >> >= 1;
     return i;
   }
 }

This adds both forms of logical right shift to the FormatTokenLexer, so clang-format won't attempt to split them and insert bogus whitespace.

Diff Detail

Repository
rL LLVM

Event Timeline

bradfier created this revision.Apr 4 2017, 2:44 AM
bradfier updated this revision to Diff 94334.Apr 6 2017, 2:30 AM
bradfier added a reviewer: klimek.

Add more diff context, add klimek as reviewer.

Are the <<< and >>> operators handled correctly?

arphaman added inline comments.Apr 6 2017, 4:24 AM
include/clang/Basic/LangOptions.def
95 ↗(On Diff #94334)

I don't think we should have a Java lang option, since Clang is not actually a Java frontend. Maybe another option like LogicalShiftOperators will work better?

Are the <<< and >>> operators handled correctly?

Yes, as a side-effect of CUDA kernel-call syntax which uses >>> and <<<, those tokens are recognised as punctuators.

bradfier added inline comments.Apr 6 2017, 4:59 AM
include/clang/Basic/LangOptions.def
95 ↗(On Diff #94334)

That seems reasonable. C and C++ have >>> or <<< operators as part of CUDA extensions though, so maybe the full LogicalShiftAssignOperators is more precise?

thakis added a subscriber: thakis.Apr 6 2017, 6:41 AM
thakis added inline comments.
include/clang/Basic/TokenKinds.def
230 ↗(On Diff #94334)

I think this is the wrong approach to go about this. clang is a C compiler, and its tokenizer shouldn't have to know about Java. Instead, this should be handled in the formatter. See tryMergePreviousTokens() in lib/Format/FormatTokenLexer.cpp for how we do this for e.g. => or >>>= etc for JavaScript – just do the same for Java.

bradfier updated this revision to Diff 94438.Apr 6 2017, 2:22 PM
bradfier edited the summary of this revision. (Show Details)

Switch to a more appropriate (and much simpler) method of identifying these Java-specific operators.

Also removed any references to fictitious "logical left shifts", I think I made those up while tired...

Thanks for that @thakis, that's a much better solution than the first attempt!

Friendly ping.

thakis accepted this revision.Apr 11 2017, 8:19 AM

Looks good. Do you have commit access?

This revision is now accepted and ready to land.Apr 11 2017, 8:19 AM

Looks good. Do you have commit access?

Thanks! And no I don't have commit access.