Page MenuHomePhabricator

[clang-format] Rebased on master: Add option to specify explicit config file
AcceptedPublic

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

Details

Reviewers
djasper
ioeric
krasimir
MyDeveloperDay
Group Reviewers
Restricted Project
Summary

Rebase original diff of https://reviews.llvm.org/D33560 onto master. I also tried to address the remaining comments.
This is a quick attempt to get this feature into clang-format - I hope that the tiny restructuring (new function) is acceptable in this form: comment & improvements welcome!

Diff Detail

Repository
rC Clang

Event Timeline

tnorth created this revision.Jan 7 2020, 5:39 AM
tnorth retitled this revision from Rebased on master: Add option to specify explicit config file to [clang-format] Rebased on master: Add option to specify explicit config file.
tnorth edited the summary of this revision. (Show Details)
tnorth added reviewers: djasper, ioeric, krasimir.

In principle, I think this is something that might help a lot of people as I've seen people asking for a better mechanism to be able to load a centrally stored .clang-format file.

but maybe wait a couple of days for others to comment @klimek , @mitchell-stellar , @sammccall ...

Also you need to make full context diffs, plus please add a release note, and because I can't check the full diff, I'm unsure if your Format.h change means we need to regenerate anything in the ClangFormatStyleOptions.rst

In this patch, relative paths are relative to the working directory (or at least the current directory of the VFS).

This makes sense for command-line args, but if I understand correctly this patch will also allow BasedOnStyle: file:some/path. Is that the case?
In that case, interpreting paths relative to the working directory seems suspicious. I'd suggest one of:

  • BasedOnStyle: file:... is not allowed
  • BasedOnStyle: file:... must be an absolute path (note this probably means it's not portable across OSs)
  • BasedOnStyle: file:relative/path is relative to the current config file, or working directory if the current config file is inline in command line args.
include/clang/Format/Format.h
2329

Please upload using full context (e.g. with arc diff)

2332

why does this need to be a public API?

2343

this function isn't implemented - LoadConfigFile vs loadConfigFile

lib/Format/Format.cpp
2573

Expected<FormatStyle> or ErrorOr<FormatStyle> would be a better return type for this function I think.
Read isn't going to return unsuitable, so plumbing through multiple error codes doesn't seem worthwhile.

2613

just slice the stringref rather than copy here?

2615

why stat the file and check, rather than just try to read it?

unittests/Format/FormatTest.cpp
14370

Can you add this via a real absolute path, to show this is actually relative path handling (and not just naive string manipulation by memory file system etc)

tnorth updated this revision to Diff 238513.Jan 16 2020, 8:46 AM

Thanks for your time and feedback.

  • Update diff with context,
  • remove public declaration of LoadConfigFile,
  • change prototype of LoadConfigFile and simplify it,
  • avoid string copy to ConfigFile, use StringRef directly,
  • don't stat the file before attempting to read it.

Regarding the tests, I am unsure how to do this currenlty (I'd need to read some docs/other examples).
I didn't have in mind the use case of BasedOnStyle: file:some/path.

Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2020, 8:46 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MyDeveloperDay added a project: Restricted Project.Jan 19 2020, 8:04 AM
tnorth updated this revision to Diff 240888.Tue, Jan 28, 7:40 AM
  • Add a unit-test loading a format and test file from temporary files.
tnorth marked 7 inline comments as done.Tue, Jan 28, 7:46 AM

This makes sense for command-line args, but if I understand correctly this patch will also allow BasedOnStyle: file:some/path. Is that the case?

No, it should not, and I also think it's better not to.

I think that all points are addressed now. Looking forward to have your feedback.

Nit: please add a release note and regenerate the ClangFormatStyleOptions.rst (if there are any changes because you modified Format.h).

tnorth updated this revision to Diff 241155.Wed, Jan 29, 7:14 AM
  • Add release notes
  • Update ClangFormat.rst and ClangFormatStyleOption.rst

Nit: please add a release note and regenerate the ClangFormatStyleOptions.rst (if there are any changes because you modified Format.h).

Hmm, I tried to run docs/tools/dump_format_style.py, but it fails at two locations (once because a comment in Format.h has two slashes instead of 3, one because of an empty line). After fixing those, it seems that the generated file contains less information than the commited one. This file was probably updated manually as well. Therefore I also did in the updated diff... one should probably fix this separately.
Release note + ClangFormat.rst files update in the diff as well.

MyDeveloperDay accepted this revision.Wed, Feb 5, 12:11 PM

In the absence of any other comments, This LGTM, this may help to resolve D68569: [clang-format] Also look for .{ext}.clang-format file, I think I prefer this solution.

This revision is now accepted and ready to land.Wed, Feb 5, 12:11 PM

Ping. I guess we need more approval to go forward?

lebedev.ri added inline comments.
include/clang/Format/Format.h
2341

So is it : or =?

tnorth updated this revision to Diff 245592.Thu, Feb 20, 12:15 AM

Fix comment to file:<configfile> instead of the incorrect file=<configfile>

tnorth marked 2 inline comments as done.Thu, Feb 20, 12:16 AM
tnorth added inline comments.
include/clang/Format/Format.h
2341

: is correct, fixed.

tnorth marked an inline comment as done.Thu, Feb 20, 12:43 AM