Page MenuHomePhabricator

clang-format: Support the Java 8 'default' method modifier
AcceptedPublic

Authored by lhchavez on Dec 2 2016, 5:02 PM.

Details

Summary

Java 8 introduced the use of using the 'default' keyword as modifier in
interface method declarations[1]. Previously it was being parsed as
being part of a label, which put the parser into a very weird state it
could not get out of.

This change adds support for 'default' by treating it as a normal
identifier in Java when the parser is expecting a declaration.

1: http://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html

Event Timeline

lhchavez updated this revision to Diff 80155.Dec 2 2016, 5:02 PM
lhchavez retitled this revision from to clang-format: Support the Java 8 'default' method modifier.
lhchavez updated this object.
lhchavez added a reviewer: djasper.
lhchavez added subscribers: srhines, cfe-commits.

Looks good, but I I want to make sure that someone else more familiar with this is ok with it too. Thanks.

Gentle ping?

lodato resigned from this revision.Dec 13 2016, 11:22 AM
lodato removed a reviewer: lodato.

I know nothing about the C++ code. I only know the git-clang-format script.

djasper added inline comments.Dec 13 2016, 11:28 AM
lib/Format/UnwrappedLineParser.cpp
297

Same as below.

865

Change the order here. I.e.

if (Style.Language == FormatStyle::LK_Java && Line->MustBeDeclaration)
  break;
...

I think then you almost don't even need the comment.

870

Is there a test case that hits this codepath? IIUC, it would need to have a "default" of a declaration that's not at the start of the line.

lhchavez updated this revision to Diff 81280.Dec 13 2016, 12:29 PM

Addressed feedback

lhchavez marked 3 inline comments as done.Dec 13 2016, 12:30 PM
lhchavez added inline comments.
lib/Format/UnwrappedLineParser.cpp
870

Added two more test cases.

djasper accepted this revision.Dec 13 2016, 10:52 PM
djasper edited edge metadata.

Looks good. Thank you!

This revision is now accepted and ready to land.Dec 13 2016, 10:52 PM

While reviewing old revisions either not landed or unreviewed I stumbled across this accepted review. Looking at the current code I think its not needed anymore. Perhaps if we merge your test in with the current trunk we can determine if this is still needed

MyDeveloperDay added a project: Restricted Project.