Page MenuHomePhabricator

[clang-format] Make '.clang-format' variants finding a loop
ClosedPublic

Authored by wanders on Oct 7 2019, 4:09 AM.

Details

Summary

No functional change intended

Diff Detail

Event Timeline

wanders created this revision.Oct 7 2019, 4:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 4:09 AM
MyDeveloperDay added a comment.EditedOct 7 2019, 4:54 AM

Thank you for the patch,

I think this looks cleaner and generally LGTM, I think its always good to put a little description of the change along with the NFC in the title to let people know that functionally there isn't any change without them having to work through the code change. Maybe even giving a little rationale as to why you are thinking of making the change.. (which I see from D68569: [clang-format] Also look for .{ext}.clang-format file)

As I understand it, you have changed the code to read the configs from a list of file in a loop rather than reading one and then trying another if that cannot be found, subsequently, this would make it relatively easy to add higher priority search options without confusing the code

I think there is some commonality here that could be used with another suggestion which was to allow --style=<filename> D50147: clang-format: support external styles

MyDeveloperDay accepted this revision.Oct 7 2019, 4:55 AM
This revision is now accepted and ready to land.Oct 7 2019, 4:55 AM

Do you need help landing this?

Do you need help landing this?

Sorry that I havn't gotten back to this.

I started looking into the "PathMatch" idea I floated in D68569 ([clang-format] Also look for .{ext}.clang-format file). That would mean something entirely different than the patch in D68596, making this refactor less needed.

But I can still land it if you think it still relavant on its own.

But I can still land it if you think it still relevant on its own.

I think the design of working through the vector of files is more elegant, especially if we want to consider looking for other files in the future.

This revision was automatically updated to reflect the committed changes.