Page MenuHomePhabricator

[clang][Format] Evaluate FallbackStyle only if needed
Needs RevisionPublic

Authored by kadircet on Jan 27 2021, 9:17 AM.

Details

Summary

Currently getStyle() fails immediately if FallbackStyle is not a
predefined style, even when it is not needed. This patch moves that evaluation
and bail-out to fallback-case, enabling getStyle() to succeed in cases where
FallbackStyle is malformed but not needed.

Diff Detail

Event Timeline

kadircet requested review of this revision.Jan 27 2021, 9:17 AM
kadircet created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2021, 9:17 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MyDeveloperDay requested changes to this revision.Jan 27 2021, 2:53 PM

This needs a unit test, I personally don't understand what this is doing or why its needed, if its a bug can you reference a bug in bugzilla that explains the bug a little clearer please

This revision now requires changes to proceed.Jan 27 2021, 2:53 PM

Sure adding tests, sorry for leaving them out the first time.

To give more insights, format::getStyle() tries to figure out FallbackStyle at the beginning of the function, and errors out if it can't. But in most cases FallbackStyle is not needed, e.g. there's already a .clang-format file in parent directories and StyleName is "file", or StyleName is a predefined/custom one.
This patch changes getStyle()s behaviour to succeed in such cases, even with an invalid FallbackStyle. As for a real world sample you can check https://github.com/clangd/coc-clangd/issues/39#issuecomment-703189067.

kadircet updated this revision to Diff 319765.Jan 27 2021, 10:39 PM
  • Add tests.

Why is fixing (or removing) the fallback style in the .clang-format file not enough??

curdeius requested changes to this revision.Jan 27 2021, 11:21 PM
This revision now requires changes to proceed.Jan 27 2021, 11:21 PM

Why is fixing (or removing) the fallback style in the .clang-format file not enough??

sure that would be one way to go, and that's what the user did already.
It feels confusing to me that getStyle() fails when provided with a malformed FallbackStyle. Even if it is not going to be returned. Are there any workflows that benefit from this behaviour?

I see little value in not checking FallbackStyle (even if it's not used).
However, I do see value in an early warning (error) on an incorrect style that can pop up later if not caught soon enough.

I return the question, are there any workflows that would benefit from the proposed behaviour? Apart from those that have already a faulty config?

I see little value in not checking FallbackStyle (even if it's not used).

note that this patch doesn't disable fallbackstyle checking, it is still checked, but not eagerly.

However, I do see value in an early warning (error) on an incorrect style that can pop up later if not caught soon enough.

i don't follow the value proposition here. if user has a valid way to provide "style", the fallbackstyle will never be used (malformed or otherwise).
so in such a case, user is currently going to get a "hard error" preventing them from formatting with the style they provided whenever they have a malformed fallback-style.

what i am trying to say is, "earliness" of the warning/error in such a scenario doesn't seem to benefit any user (at least to me).
since fixing their fallbackstyle won't change anything for formatting of the particular file in question, and they'll still see the error whenever they try to format a file that needs the fallbackstyle.
hence currently the eager validation of a malformed FallbackStyle is *only* preventing a user with a valid Style from formatting their file (it is erroring in all other cases, but there's nothing much to be done for those).

to add, there will never be a file formatted with the incorrect style.

I return the question, are there any workflows that would benefit from the proposed behaviour? Apart from those that have already a faulty config?

that's the exact set of workflows that this change is going to effect(if FallbackStyle is valid, this patch is a no-op). so, no, i don't think there will be any other workflows that can benefit from this change, but it feels like this is a non-empty set of users (while not regressing any users that benefit from current error/warning, as they'll still be surfaced when needed).