Page MenuHomePhabricator

[clang-format] [PR43100] clang-format C# support does not add a space between "using" and paren
ClosedPublic

Authored by MyDeveloperDay on Aug 23 2019, 8:57 AM.

Details

Summary

Addresses https://bugs.llvm.org/show_bug.cgi?id=43100

Formatting using statement in C# with clang-format removes the space between using and paren even when SpaceBeforeParens is !

using(FileStream fs = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.Read, bufferSize : 1))

this change simply overcomes this for when using C# settings in the .clang-format file

using (FileStream fs = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.Read, bufferSize : 1))

All FormatTests pass..

[==========] 688 tests from 21 test cases ran. (88508 ms total)
[  PASSED  ] 688 tests.

Diff Detail

Repository
rL LLVM

Event Timeline

MyDeveloperDay created this revision.Aug 23 2019, 8:57 AM
owenpan added inline comments.Aug 23 2019, 3:44 PM
clang/lib/Format/TokenAnnotator.cpp
2618 ↗(On Diff #216870)

if (Style.isCSharp() && Left.is(tok::kw_using)) would suffice as Right.is(tok::l_paren) is already checked on Line 2613.

clang/unittests/Format/FormatTestCSharp.cpp
169 ↗(On Diff #216870)

Maybe set SpaceBeforeParens to Always first in order to really test the new behavior?

owenpan added inline comments.Aug 23 2019, 8:28 PM
clang/unittests/Format/FormatTestCSharp.cpp
169 ↗(On Diff #216870)

I meant setting SpaceBeforeParens to Always Never.

owenpan added inline comments.Aug 24 2019, 12:32 AM
clang/unittests/Format/FormatTestCSharp.cpp
169 ↗(On Diff #216870)

Also, the right parenthesis for the using statement is missing.

owenpan added a comment.EditedAug 24 2019, 10:33 PM

The test case fails after the missing ) is added, so it seems that the patch has no effect.

MyDeveloperDay marked 4 inline comments as done.

The patch would have no effect if the using statement is not in a block, but that would probably be against C# syntax.

Please see my comments about the test cases. Otherwise, LGTM.

clang/unittests/Format/FormatTestCSharp.cpp
168 ↗(On Diff #217167)

I'd add the following as the first test case because it tests the new behavior. The other two test cases only verify that the patch does not cause regressions. (The SBPO_Always case may be redundant if it's already covered in FormatTest.cpp.)

// SpaceBeforeParens: ControlStatements
verifyFormat("public void foo() {\n"
             "  using (StreamWriter sw = new StreamWriter(filenameA)) {}\n"
             "}");
owenpan accepted this revision.Aug 26 2019, 11:55 PM
This revision is now accepted and ready to land.Aug 26 2019, 11:55 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 12 2019, 3:18 AM