Page MenuHomePhabricator

[clang-format][NFC] Allow getLLVMStyle() to take a language
ClosedPublic

Authored by rupprecht on Jan 18 2019, 3:20 PM.

Details

Summary

getLLVMStyle() sets the default style, but doesn't take the language as a parameter, so can't set default parameters when they differ from C++. This change adds LanguageKind as an input to getLLVMStyle so that we can start doing that.

See D55964 as a motivation for this, where we want Tablegen to be formatted differently than C++.

Event Timeline

rupprecht created this revision.Jan 18 2019, 3:20 PM
rupprecht set the repository for this revision to rC Clang.Jan 18 2019, 3:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2019, 3:35 PM

The patch itself looks sound. However given that you have a specific use case in mind (TableGen files) could you provide supplementary coverage for that specific use case (unit tests for formatting td syntax using format::getLLVMStyle(format::FormatStyle::LK_TableGen)? I'm not entirely sure how useful this particular change is given that there's no linked patches related to your use case, I think adding those would help as well (possibly as a separate dependent patchset).

The patch itself looks sound. However given that you have a specific use case in mind (TableGen files) could you provide supplementary coverage for that specific use case (unit tests for formatting td syntax using format::getLLVMStyle(format::FormatStyle::LK_TableGen)? I'm not entirely sure how useful this particular change is given that there's no linked patches related to your use case, I think adding those would help as well (possibly as a separate dependent patchset).

The use case is a pre-refactoring for D55964, which is in the patch description -- is that what you mean by "linked patches"?
I'll update that patch to use this approach, and make it a dependent patch.

The patch itself looks sound. However given that you have a specific use case in mind (TableGen files) could you provide supplementary coverage for that specific use case (unit tests for formatting td syntax using format::getLLVMStyle(format::FormatStyle::LK_TableGen)? I'm not entirely sure how useful this particular change is given that there's no linked patches related to your use case, I think adding those would help as well (possibly as a separate dependent patchset).

The use case is a pre-refactoring for D55964, which is in the patch description -- is that what you mean by "linked patches"?
I'll update that patch to use this approach, and make it a dependent patch.

Added as a dependent patch. I've never linked patches together, so I might not be doing it right. Guess I'll read some phab docs now...

MyDeveloperDay added inline comments.
clang/lib/Format/ContinuationIndenter.cpp
193

if you've set up a default argument of LK_Cpp do you need to keep supplying it? if your going to supply it everywhere don't make it a default argument or vice versa

This might make a much smaller patch, a little more digestable and all the tests can remain as they are.

rupprecht marked 2 inline comments as done.

Revert getLLVMStyle(LK_Cpp) fixes

rupprecht added inline comments.Feb 27 2019, 11:21 AM
clang/lib/Format/ContinuationIndenter.cpp
193

My goal was to fix all the in-tree callers but leave it as an optional arg (at least initially) so as to not break out of tree callers. I reduced it to what I actually need for this patch.

MyDeveloperDay accepted this revision.Feb 27 2019, 2:07 PM

I think I've seen you need this for another patch, so in the absence of the code owners this LGTM, and thank you for removing all the changes to the tests in the previous diff.

This revision is now accepted and ready to land.Feb 27 2019, 2:07 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2019, 11:16 AM