Page MenuHomePhabricator

[clang-format] Add option to explicitly specify a config file
ClosedPublic

Authored by zwliew on Jan 7 2020, 5:39 AM.

Details

Summary

This diff extends the -style=file option to allow a config file to be specified explicitly. This is useful (for instance) when adding IDE commands to reformat code to a personal style.

Usage: clang-format -style=file:<path/to/config/file> ...

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
zwliew added a subscriber: zwliew.Dec 23 2021, 10:13 PM

Hi, I'd like to help to get this patch accepted and merged. I have a few suggestions/questions below, and I can help make any changes to the patch if needed!

clang/docs/ClangFormatStyleOptions.rst
35

I think two backticks are missing here.

clang/include/clang/Format/Format.h
4069

To be consistent with the ClangFormatOptions.rst docs, I think configfile should be changed to format_file_path.

clang/lib/Format/Format.cpp
3184

Similar to above, configfile should be changed to format_file_path.

3237

I think it's still a good idea to pass Style into the function instead.

3237

I think LoadConfigFile could be refactored to return std::error_code instead, similar to parseConfiguration(), like so:

// note I renamed LoadConfigFile to loadConfigFile to follow the existing function naming style.
std::error_code loadConfigFile(StringRef ConfigFile, llvm::vfs::FileSystem *FS, FormatStyle *Style) {
    llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Text = FS->getBufferForFile(ConfigFile.str());
    if (auto ec = Text.getError()) {
        return ec;
    }
    return parseConfiguration(Text.get()->getBuffer(), Style);
}

And the part in getStyle() would look like this:

// Check for explicit config filename
if (StyleName.startswith_insensitive("file:")) {
    auto StyleFileName = StyleName.substr(5);
    if (auto ec = loadConfigFile(StyleFileName, FS, &Style)) {
        return make_string_error(ec.message());
    }
    LLVM_DEBUG(llvm::dbgs() << "Using configuration file " << StyleFileName << '\n');
    return Style;
}

What do you think?

3280

Yup, I think this shouldn't be moved.

3283

Following my code suggestion above, I think this code block could be refactored to this:

// Check for explicit config filename
if (StyleName.startswith_insensitive("file:")) {
    auto StyleFileName = StyleName.substr(5);
    if (auto ec = loadConfigFile(StyleFileName, FS, &Style)) {
        return make_string_error(ec.message());
    }
    LLVM_DEBUG(llvm::dbgs() << "Using configuration file " << StyleFileName << '\n');
    return Style;
}

What do you think?

Also, on another note, I feel like the -style option is too overloaded. Would it be better to use a separate option like -style-file instead?

zwliew added a comment.EditedDec 24 2021, 12:40 AM

On further thought, the logic for loadConfigFile() looks incomplete. It does not properly handle BasedOnStyle: InheritParentConfig. In fact, the logic for -style=file can be pulled out and merged into loadConfigFile().

I'll look into making this change, and possibly refactoring the code to reduce code duplication.

MyDeveloperDay marked 4 inline comments as done.Dec 24 2021, 5:59 AM

Please go ahead

zwliew commandeered this revision.Dec 26 2021, 10:12 PM
zwliew added a reviewer: MyDeveloperDay.
zwliew updated this revision to Diff 396273.EditedDec 26 2021, 10:20 PM

Support inheritance from parent configs via BasedOnStyle: InheritParentConfig

I made the following changes:

  1. Don't return early when BasedOnStyle: InheritParentConfig is set. Also search for parent configs starting from the parent directory of the specified config file.
  1. Remove the LoadConfigFile() function. There isn't a clean way to refactor the --style=file logic to use it and The function is small and only used in 1/2 places, so I don't think is much of a reason to keep it.

Please have a look and let me know your thoughts, thanks! Also, this is my first time using Phabricator, so let me know if I did anything wrong.

Side question: Is there a reason that the number of children configs in the fallback case is limited to 1 (that is, when all the configs have BasedOnStyle: InheritParentConfig set)?

zwliew added inline comments.Dec 26 2021, 10:30 PM
clang/lib/Format/Format.cpp
3401–3402

Is there a reason for this to be limited to 1? I came across this case during testing, but I can't think of why this should be so.

zwliew updated this revision to Diff 396274.Dec 26 2021, 10:45 PM

Add a test for inheritance from parent config with base

clang/lib/Format/Format.cpp
3283

You can certainly refactor code.

What happens with -style="Google" -style-file="Foo/Bar" is it different from -style-file="Foo/Bar" -style="Google"?
I do not perse disagree, but I think it makes stuff clearer if there is only one option.

3401–3402

See comment on D93844.

zwliew added inline comments.Dec 27 2021, 7:35 AM
clang/lib/Format/Format.cpp
3283

Fair enough. Some ideas off the top of my head that resolve the ambiguity sound needlessly complicated. Thanks!

Do you plan to refactor something for this review, or do you think you are done? Then I will look at it again as a whole.

Do you plan to refactor something for this review, or do you think you are done? Then I will look at it again as a whole.

I'm going to try refactoring the code for loading and parsing the config file into a separate function. I'll update the diff in no longer than a day.

zwliew updated this revision to Diff 396458.EditedDec 28 2021, 7:51 PM

I am done. Please review the latest changes. Thanks!

I made the following changes:

  1. Refactor the code for loading and parsing configs into a separate function
  1. Add a new test case (9.1.2) to test the case mentioned in https://reviews.llvm.org/D93844#inline-1112285.
  1. Fix the test case failure for the newly added test 9.1.2.
zwliew added inline comments.Dec 28 2021, 7:54 PM
clang/lib/Format/Format.cpp
3283

I've refactored the code in the latest revision (under loadAndParseConfigFile().

3401–3402

I've added a new test case (9.1.2) to illustrate the test failure, and also fixed the code to fix the test case.

curdeius accepted this revision.Dec 29 2021, 2:51 AM

LGTM but please wait for the green light from other involved reviewers.
Also, as noted in a comment, I'd like to see unrelated changes in a different patch (unless you convince me that it's overly complicated or too much dependent on this patch).

clang/lib/Format/Format.cpp
3298

Here and elsewhere: please end the comments with a full stop.

clang/unittests/Format/FormatTest.cpp
21466–21467

?

21483

Can we split changes unrelated to the specific configuration file (fixing/testing the inherited config) to another patch?

This revision is now accepted and ready to land.Dec 29 2021, 2:51 AM

Oh, and please rename the patch before landing.

zwliew updated this revision to Diff 396519.Dec 29 2021, 5:29 AM
zwliew retitled this revision from [clang-format] Rebased on master: Add option to specify explicit config file to [clang-format] Add option to explicitly specify a config file.
zwliew edited the summary of this revision. (Show Details)

Addressed the comments on the previous diff:

  1. Adjusted some comments; suffixed each comment with a period.
  2. Removed the changes unrelated to this revision (the ones the issue mentioned in the latest comments in https://reviews.llvm.org/D93844)
zwliew edited the summary of this revision. (Show Details)Dec 29 2021, 5:29 AM

Thanks for the review. I've moved the unrelated change to https://reviews.llvm.org/D116371 instead.

Although there is no context. Maybe still update the uploaded diff for the archive.

zwliew updated this revision to Diff 396578.Dec 29 2021, 5:00 PM

Rebased on master for context

Herald added a reviewer: andreadb. · View Herald Transcript
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: sjarus. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
This revision now requires review to proceed.Dec 29 2021, 5:00 PM

Oh no...I'm so sorry. Didn't mean to cause this large diff. Trying to fix it now.

zwliew updated this revision to Diff 396579.EditedDec 29 2021, 5:07 PM

Fixed rebase and diff, and tried to restore the reviewers and subscribers.

zwliew removed projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project.
zwliew removed subscribers: JDevlieghere, arsenm, dschuff and 98 others.
This revision is now accepted and ready to land.Dec 29 2021, 5:11 PM

Do you have commit access, or do you need someone to commit this and your other changes? If the latter we need a name and an email for the commit. Then someone will come along and commit it.

zwliew accepted this revision.Dec 30 2021, 6:39 PM

Do you have commit access, or do you need someone to commit this and your other changes? If the latter we need a name and an email for the commit. Then someone will come along and commit it.

I don't think I have commit access, so I would be grateful if someone helps me do it.

Name:
Zhao Wei Liew

Email:
zhaoweiliew@gmail.com

This revision was landed with ongoing or failed builds.Jan 3 2022, 2:43 AM
This revision was automatically updated to reflect the committed changes.
fdwr awarded a token.Jan 25 2022, 9:59 PM