This is an archive of the discontinued LLVM Phabricator instance.

[clang-format][NFC] Give State.Stack.back() a meaningful name
ClosedPublic

Authored by HazardyKnusperkeks on Feb 11 2022, 2:54 PM.

Details

Diff Detail

Event Timeline

HazardyKnusperkeks requested review of this revision.Feb 11 2022, 2:54 PM
HazardyKnusperkeks created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2022, 2:54 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
owenpan added inline comments.Feb 12 2022, 12:16 AM
clang/lib/Format/ContinuationIndenter.cpp
1514

Can you make it const here?

curdeius accepted this revision.Feb 12 2022, 12:19 AM

LGTM whether you keep it as is or use a pointer.
It looks like it would be cool if we had some sort of expression aliases in the language... using CurrentState = State.Stack.back()... One can always dream... :)

clang/lib/Format/ContinuationIndenter.cpp
1356

Maybe it would be better to use a pointer instead of a reference and then you could rebind it to the need back()?

This revision is now accepted and ready to land.Feb 12 2022, 12:19 AM
HazardyKnusperkeks planned changes to this revision.Feb 12 2022, 1:02 AM
HazardyKnusperkeks added inline comments.
clang/lib/Format/ContinuationIndenter.cpp
1356

I'm not a fan of using pointers for such things. I would've done that, if the remaining function was longer or we had more calls to the state.

1514

I think yeah I could. This is maybe an remainder of some of my tries to get the requires stuff working, that I would've changed the current state here.

I like this change for clarity reasons, my only concern and it's not based on evidence is what if any of these functions get passed in State, and then they themselves alter the State.Stack?

In most cases I'd expect CurrentState to always be correct, but doesn't it only represent the state of the stack as it was at the beginning of the function

Isn't the point of State.Stack.back() to get the current state? if you do that with a variable like CurrentState or just have State.getCurrentState() I don't know but the later seems a good step because you could use that in even more places

i.e.

unsigned NestedBlockIndent = State.Stack.back().NestedBlockIndent;

unsigned NestedBlockIndent = State.getCurrentState().NestedBlockIndent;

I like this change for clarity reasons, my only concern and it's not based on evidence is what if any of these functions get passed in State, and then they themselves alter the State.Stack?

In most cases I'd expect CurrentState to always be correct, but doesn't it only represent the state of the stack as it was at the beginning of the function

Isn't the point of State.Stack.back() to get the current state? if you do that with a variable like CurrentState or just have State.getCurrentState() I don't know but the later seems a good step because you could use that in even more places

i.e.

unsigned NestedBlockIndent = State.Stack.back().NestedBlockIndent;

unsigned NestedBlockIndent = State.getCurrentState().NestedBlockIndent;

Currently everywhere where I have created the reference there are no changes to the stack afterwards. If someone will change that in the future they will get massive test failures, that I can promise from experience in this field.
My reason for this change was debugging, I had a really hard time to understand what all these values and their changes (and why they change) mean. With the local variable I could easily monitor the values in my debugger view. If we replace one function call State.Stack.back() with State.getCurrentState() we are back at square one.

@HazardyKnusperkeks agreed, I'm fine with this approach (especially on what is one of the more confusing parts of clang-format)

HazardyKnusperkeks marked an inline comment as done.

Added const

This revision is now accepted and ready to land.Feb 12 2022, 4:40 PM
owenpan accepted this revision.Feb 12 2022, 10:04 PM
MyDeveloperDay accepted this revision.Feb 13 2022, 1:50 AM