Page MenuHomePhabricator

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

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

Details

Reviewers
djasper
krasimir
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 TranscriptWed, Feb 6, 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...