Page MenuHomePhabricator

[clang-format] Add more support for C# 8 nullables
ClosedPublic

Authored by exv on May 1 2021, 3:11 PM.

Diff Detail

Event Timeline

exv requested review of this revision.May 1 2021, 3:11 PM
exv created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2021, 3:11 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thank you for the patch

clang/lib/Format/TokenAnnotator.cpp
3170

should this honour SpaceBeforeAssignmentOperators ?

exv updated this revision to Diff 342497.May 3 2021, 12:09 PM

Remove special whitespace handling for ??=

clang/lib/Format/TokenAnnotator.cpp
3170

Good point. Looking more closely, since I have called setKind(tok::equal) on the operator, it seems that the rest of clang-format reliably understands it as an assignment operator and formats it correctly if I don't intervene. I have updated the test case to test this behavior.

exv updated this revision to Diff 342520.May 3 2021, 1:06 PM

Fix incorrect arc usage

Herald added a reviewer: rriddle. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
exv updated this revision to Diff 342521.May 3 2021, 1:09 PM

Fixing arc, hopefully?

exv added a comment.May 3 2021, 1:11 PM

Hey all, I'm really sorry for the noise, I screwed up my arc command to revise the submission.

exv removed reviewers: andreadb, rriddle, aartbik, Restricted Project.May 3 2021, 1:17 PM
exv edited projects, added Restricted Project; removed Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project.
exv removed subscribers: JDevlieghere, jholewinski, emaste and 63 others.
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2021, 1:17 PM
HazardyKnusperkeks added inline comments.
clang/unittests/Format/FormatTestCSharp.cpp
365

Why do you escape ?? I've never seen that.

exv added inline comments.May 3 2021, 1:34 PM
clang/unittests/Format/FormatTestCSharp.cpp
365

Apparently, ??= is a trigraph for "#" in C, so it must be escaped:

https://en.wikipedia.org/wiki/Digraphs_and_trigraphs#C

To be honest, I'd never heard of this bizarre language "feature" until I wrote this patch.

curdeius requested changes to this revision.May 3 2021, 2:25 PM
curdeius added a subscriber: curdeius.

Looks very good already. Just some questions and test requests.

Requesting changes so it shows up nicely in the revision queue.

clang/lib/Format/TokenAnnotator.cpp
1578

Does adding false and true here may negatively affect JS in any way? (Just thinking out loud)

3168

Should it be renamed to include C#?

clang/unittests/Format/FormatTestCSharp.cpp
365

Not only in C but in C++ too. Fortunately removed in C++17.
Nobody misses trigraphs...

380

Please test also true! or !true!.

381

Could you test that ! is correctly kept together with the corresponding identifier around the line breaks, please?
You might adjust ColumnLimit for that.

This revision now requires changes to proceed.May 3 2021, 2:25 PM
exv updated this revision to Diff 342594.May 3 2021, 4:46 PM
exv marked 2 inline comments as done.
  • [clang-format] Add more support for C# 8 nullables
  • Fix null-coalescing assignment whitespace behavior
  • Incorporate feedback from curdeius
  • Combine JS and CS types/parsing
clang/lib/Format/TokenAnnotator.cpp
1578
3168

Now that you've mentioned it, I do see that JS also defines an almost duplicate set of null-related operators, including null-propagating, null-coalescing and null-coalescing assignment operators, and clang-format handles them with a different code path than C# 😅. They do have slightly different names, but the way they should be parsed and interpreted is the same, and I think it makes sense to use the same logic and token definitions for both of these languages. This resulted in a lot of special-handling for C# tokens that wasn't necessary being removed too.

However, renaming them would introduce a lot of noise, so I think I would prefer to do that in a separate commit.

exv marked 4 inline comments as done.May 3 2021, 4:50 PM
curdeius accepted this revision.May 4 2021, 12:14 AM

LGTM. That's great. Thanks for working on this and for the cleanup.

clang/lib/Format/FormatTokenLexer.cpp
102

Could you explain in what case the operator precedence may be wrong here?

clang/lib/Format/TokenAnnotator.cpp
3168

Fair enough.

This revision is now accepted and ready to land.May 4 2021, 12:14 AM
exv updated this revision to Diff 342813.May 4 2021, 11:52 AM
exv marked an inline comment as done.
  • Remove obsolete comment
exv marked an inline comment as done.May 4 2021, 11:53 AM

According to the wiki, it seems like someone who has commit access must now submit this change.

clang/lib/Format/FormatTokenLexer.cpp
102

It's unlikely this comment is still relevant for this section. I will delete it.

According to the wiki, it seems like someone who has commit access must now submit this change.

Yes, I'll land it. Please provide "Name <mail@address>" for commit attribution if it differs from the info in your Phabricator account.

exv marked an inline comment as done.May 4 2021, 12:18 PM

Eliza Velasquez <exv@google.com>

exv added a comment.May 4 2021, 2:04 PM

I have completed two other formatting fixes related to C# 8 nullables since my last commit. Should I create a new review request for those or update this one?

Preferably a new one unless it's something really minor.

Could you please rebase to re-run the CI (that failed for sum unrelated reason)?

krasimir accepted this revision.May 5 2021, 2:34 AM
krasimir added inline comments.
clang/lib/Format/TokenAnnotator.cpp
1570

nit: s/isIdentifier/IsIdentifier/ (LLVM style uses CamelCase for local variables)

clang/unittests/Format/FormatTestCSharp.cpp
365

nit: you could use raw string literals for the ??= test cases (e.g., R"(test ??= ABC;)").

krasimir added inline comments.May 5 2021, 2:41 AM
clang/lib/Format/FormatTokenLexer.cpp
135

nit: use spaces for indentation, no tabs (Phabricator makes it look this line starts with a tab character)

jbcoe added a subscriber: jbcoe.May 5 2021, 3:51 AM

Thanks for this patch!

clang/lib/Format/FormatTokenLexer.cpp
135

That's not a tab, I fell for that one too once. It just visualizes that there was only whitespace added.

exv updated this revision to Diff 343142.May 5 2021, 12:26 PM
exv marked 3 inline comments as done.
  • Incorporate krasimir's feedback
exv added a comment.May 5 2021, 12:27 PM

Rebased and comments addressed!

exv edited the summary of this revision. (Show Details)May 5 2021, 12:28 PM
exv updated this revision to Diff 343177.May 5 2021, 1:53 PM

Rebase again in an attempt to fix CI

This revision was automatically updated to reflect the committed changes.