This is an archive of the discontinued LLVM Phabricator instance.

Differential D75675

[clang-format] do not insert spaces around inline asm symbolic names
ClosedPublic

Authored by krasimir on Mar 5 2020, 4:34 AM.

Details

Reviewers
hans
Commits
rG36c2ab8d04cd: [clang-format] do not insert spaces around inline asm symbolic names
Summary

Fixes https://bugs.llvm.org/show_bug.cgi?id=45108.

The [ in such cases was mis-annotated as an TT_ArrayInitializerLSquare.

Diff Detail

Event Timeline

krasimir created this revision.Mar 5 2020, 4:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2020, 4:34 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
krasimir edited the summary of this revision. (Show Details)Mar 5 2020, 4:37 AM
krasimir added a reviewer: hans.
hans added a comment.Mar 5 2020, 4:49 AM

Very nice, thanks!

clang/lib/Format/TokenAnnotator.cpp
502

Is the Style.isCpp() check necessary here?

If it's necessary, maybe that's because the setting of InsideInlineASM should be guarded by Style.isCpp() instead?

krasimir updated this revision to Diff 248445.Mar 5 2020, 4:57 AM
krasimir marked an inline comment as done.

Remove redundant check.

krasimir marked an inline comment as done.Mar 5 2020, 4:58 AM
krasimir added inline comments.
clang/lib/Format/TokenAnnotator.cpp
502

Good point! I think it's unnecessary.

hans accepted this revision.Mar 5 2020, 4:59 AM

lgtm, thanks for the quick fix!

This revision is now accepted and ready to land.Mar 5 2020, 4:59 AM
krasimir updated this revision to Diff 248449.Mar 5 2020, 5:16 AM

Merging commits into 1 patch

Harbormaster completed remote builds in B48175: Diff 248438.Mar 5 2020, 5:29 AM
Closed by commit rG36c2ab8d04cd: [clang-format] do not insert spaces around inline asm symbolic names (authored by krasimir). · Explain WhyMar 5 2020, 5:29 AM
This revision was automatically updated to reflect the committed changes.
Harbormaster completed remote builds in B48180: Diff 248445.Mar 5 2020, 6:02 AM
Harbormaster completed remote builds in B48182: Diff 248449.
MyDeveloperDay added a project: Restricted Project.Mar 5 2020, 7:15 AM

Revision Contents

PathSize
clang/
lib/
Format/
2 lines

Diff 248445

clang/lib/Format/TokenAnnotator.cpp

Show First 20 Lines • Show All 493 Lines • ▼ Show 20 Lines bool parseSquare() {
bool ColonFound = false; bool ColonFound = false;
unsigned BindingIncrease = 1; unsigned BindingIncrease = 1;
if (IsCppStructuredBinding) { if (IsCppStructuredBinding) {
Left->Type = TT_StructuredBindingLSquare; Left->Type = TT_StructuredBindingLSquare;
} else if (Left->is(TT_Unknown)) { } else if (Left->is(TT_Unknown)) {
if (StartsObjCMethodExpr) { if (StartsObjCMethodExpr) {
Left->Type = TT_ObjCMethodExpr; Left->Type = TT_ObjCMethodExpr;
} else if (Style.isCpp() && InsideInlineASM) { } else if (InsideInlineASM) {
hansUnsubmitted
Done
ReplyInline Actions

Is the Style.isCpp() check necessary here?

If it's necessary, maybe that's because the setting of InsideInlineASM should be guarded by Style.isCpp() instead?

hans: Is the Style.isCpp() check necessary here? If it's necessary, maybe that's because the setting…
krasimirAuthorUnsubmitted
Done
ReplyInline Actions

Good point! I think it's unnecessary.

krasimir: Good point! I think it's unnecessary.
Left->Type = TT_InlineASMSymbolicNameLSquare; Left->Type = TT_InlineASMSymbolicNameLSquare;
} else if (IsCpp11AttributeSpecifier) { } else if (IsCpp11AttributeSpecifier) {
Left->Type = TT_AttributeSquare; Left->Type = TT_AttributeSquare;
} else if (Style.Language == FormatStyle::LK_JavaScript && Parent && } else if (Style.Language == FormatStyle::LK_JavaScript && Parent &&
Contexts.back().ContextKind == tok::l_brace && Contexts.back().ContextKind == tok::l_brace &&
Parent->isOneOf(tok::l_brace, tok::comma)) { Parent->isOneOf(tok::l_brace, tok::comma)) {
Left->Type = TT_JsComputedPropertyName; Left->Type = TT_JsComputedPropertyName;
} else if (Style.isCpp() && Contexts.back().ContextKind == tok::l_brace && } else if (Style.isCpp() && Contexts.back().ContextKind == tok::l_brace &&
▲ Show 20 Lines • Show All 3,378 Lines • Show Last 20 Lines