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

curdeius added a comment. Oh, and please rename the patch before landing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 ___ cfe-commits mailing list

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

curdeius accepted this revision. curdeius added a comment. This revision is now accepted and ready to land. 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

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

zwliew added inline comments. Comment at: clang/lib/Format/Format.cpp:3393 if (!ChildFormatTextToApply.empty()) { assert(ChildFormatTextToApply.size() == 1); HazardyKnusperkeks wrote: > zwliew wrote: > > Is there a reason for this to be limited to 1? I

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

zwliew updated this revision to Diff 396458. zwliew added a comment. Support inheritance in a chain of more than 1 parents I made the following changes: 1. Refactor the code for loading and parsing configs into a separate function 2. Add a new test case (9.1.2) to test the case mentioned in

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

zwliew added a comment. In D72326#3211701 , @HazardyKnusperkeks wrote: > 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

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

HazardyKnusperkeks added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326

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

zwliew added inline comments. Comment at: clang/lib/Format/Format.cpp:3276 + // Check for explicit config filename + if (StyleName.startswith_insensitive("file:")) { +auto StyleNameFile = StyleName.substr(5); HazardyKnusperkeks wrote: > zwliew wrote: > >

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

HazardyKnusperkeks added inline comments. Comment at: clang/lib/Format/Format.cpp:3393 if (!ChildFormatTextToApply.empty()) { assert(ChildFormatTextToApply.size() == 1); zwliew wrote: > Is there a reason for this to be limited to 1? I came across this

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

zwliew updated this revision to Diff 396274. zwliew added a comment. Add a test for inheritance from parent config with base Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 Files:

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

zwliew added inline comments. Comment at: clang/lib/Format/Format.cpp:3393 if (!ChildFormatTextToApply.empty()) { assert(ChildFormatTextToApply.size() == 1); Is there a reason for this to be limited to 1? I came across this case during testing, but I

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

zwliew updated this revision to Diff 396273. zwliew added a comment. Support inheritance from parent configs via `BasedOnStyle: InheritParentConfig` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 Files:

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

MyDeveloperDay marked 4 inline comments as done. MyDeveloperDay added a comment. Please go ahead CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

zwliew added a comment. On further thought, the logic for `loadConfigFile()` looks incomplete. It does not properly handle the `InheritParentConfig` argument for `BasedOnStyle`. In fact, `loadConfigFile()` should probably use the same logic as that for `-style=file`. I can look into making

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

zwliew added a comment. 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! Comment at: clang/docs/ClangFormatStyleOptions.rst:35 +When using ``-style=file:,

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

HazardyKnusperkeks added inline comments. Comment at: clang/lib/Format/Format.cpp:3231 +return Style; + } else if (ParserErrorCode != ParseError::Success) { +return make_string_error("Error reading " + ConfigFile + ": " + else after return But I prefer

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

MyDeveloperDay added a comment. Ping! Could we see this previously approved patch over the line I’d like to use it to build a regression suit of code and format files without the need for every test to be in its own directory CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/

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

tnorth added a comment. Hi there, Yes, sorry for the long silence, I ended up not having time to come back to this anymore for personal reasons. I also lack familiarity with the codebase and tests, and I couldn't invest more time into it; I had the impression that this last failing test would

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

MyDeveloperDay added a comment. The reason I have picked this us was because of: https://twitter.com/bruxisma/status/1462987809879257101 This slightly annoys me because : a) What they were talking about was in my view is disrespectful and inaccurate. b) I thought we'd already landed this

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

HazardyKnusperkeks added inline comments. Comment at: clang/lib/Format/Format.cpp:3274 + llvm::SmallVector FilesToLookFor; + // User provided clang-format file using -style=file:/path/to/format/file + // Check for explicit config filename MyDeveloperDay

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

MyDeveloperDay added inline comments. Comment at: clang/lib/Format/Format.cpp:3274 + llvm::SmallVector FilesToLookFor; + // User provided clang-format file using -style=file:/path/to/format/file + // Check for explicit config filename part of me wonders if

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

MyDeveloperDay updated this revision to Diff 389488. MyDeveloperDay added a comment. Rebasing previously landed but reverted commit CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 Files: clang/docs/ClangFormat.rst

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

MyDeveloperDay added a comment. After rebasing these tests seem to work (on Windows) -- Testing: 24 tests, 16 workers -- PASS: Clang :: Format/dry-run-alias.cpp (1 of 24) PASS: Clang :: Format/remove-duplicate-includes.cpp (2 of 24) PASS: Clang :: Format/disable-format.cpp (3 of 24)

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

MyDeveloperDay added a comment. @tnorth Are you planning on working on this or do you mind if one of us fixes the issues enough to get it over the line. (this was previously accepted and landed but the unit tests fail, for what looks like a fairly minor issue) Repository: rC Clang CHANGES

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

MyDeveloperDay added a comment. ping @tnorth are you planning on fixing this as its currently reverted. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 ___ cfe-commits

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

MyDeveloperDay requested changes to this revision. MyDeveloperDay added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/Format/Format.cpp:2693 + llvm::vfs::FileSystem *FS, +

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

MyDeveloperDay added a comment. These tests still fail running the following in the build directory (if your build directory is side-by-side with the llvm-project directory): c:/Python37/python ./bin/llvm-lit.py -v ./tools/clang/test/Format $ ./run_format_lit_tests.sh llvm-lit.py:

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

MyDeveloperDay added a comment. Were you able to rerun these tests? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

tnorth updated this revision to Diff 258029. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 Files: clang/docs/ClangFormat.rst clang/docs/ClangFormatStyleOptions.rst clang/docs/ReleaseNotes.rst

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

MyDeveloperDay added inline comments. Comment at: clang/lib/Format/Format.cpp:2704 + if (ParserErrorCode == ParseError::Unsuitable) { +*IsSuitable = false; +return Style; I think this isn't needed won't it already be false from line @2695 Repository:

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

tnorth updated this revision to Diff 257970. tnorth added a comment. Sorry. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 Files: clang/docs/ClangFormat.rst clang/docs/ClangFormatStyleOptions.rst

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

MyDeveloperDay added inline comments. Comment at: clang/lib/Format/Format.cpp:2700 + if (std::error_code EC = Text.getError()) +return make_string_error(EC.message()); + std::error_code ParserErrorCode = if you return here IsSuitable will be true..

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

tnorth added a comment. Ping? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

tnorth added a comment. Ping @MyDeveloperDay , any clue about what's going on here? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 ___ cfe-commits mailing

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

mitchell-stellar reopened this revision. mitchell-stellar added a comment. This revision is now accepted and ready to land. Reverted this commit due to an unexpected test failure: TEST 'Clang :: Format/dump-config-objc.h' FAILED Script: -- :

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG10b1a87ba35d: [clang-format] Add option to specify explicit config file Summary: This diff… (authored by mitchell-stellar). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

tnorth updated this revision to Diff 249616. tnorth added a comment. Rebased on master (6d5603e2 ). Some refactoring was indeed needed. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/

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

mitchell-stellar added a comment. It's not more approval that is needed, it's just that someone with commit access (assuming you do not) needs to find the time to commit this. For what it's worth, I'm getting a patch rejection for the 4th block in lib/Format/Format.cpp. It seems the contents

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

tnorth added a comment. Thanks @MyDeveloperDay . I guess more approvals are needed at this point? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 ___ cfe-commits mailing list

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

MyDeveloperDay accepted this revision. MyDeveloperDay added a comment. LGTM Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 ___ cfe-commits mailing list

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

tnorth added a comment. Ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

tnorth marked 2 inline comments as done. tnorth added inline comments. Comment at: include/clang/Format/Format.h:2341 /// directory if ``FileName`` is empty. +/// * "file=" to explicitly specify the configuration file to use. /// lebedev.ri wrote: > So is it

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

tnorth updated this revision to Diff 245592. tnorth added a comment. Fix comment to file: instead of the incorrect file= Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 Files: docs/ClangFormat.rst

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

lebedev.ri added inline comments. Comment at: include/clang/Format/Format.h:2341 /// directory if ``FileName`` is empty. +/// * "file=" to explicitly specify the configuration file to use. /// So is it `:` or `=`? Repository: rC Clang CHANGES SINCE LAST

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

tnorth added a comment. Ping. I guess we need more approval to go forward? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 ___ cfe-commits mailing list

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

MyDeveloperDay accepted this revision. MyDeveloperDay added a comment. This revision is now accepted and ready to land. 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

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

tnorth added a comment. Ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

tnorth updated this revision to Diff 241155. tnorth added a comment. - Add release notes - Update ClangFormat.rst and ClangFormatStyleOption.rst Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 Files:

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

tnorth added a comment. In D72326#1845342 , @MyDeveloperDay wrote: > 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

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

MyDeveloperDay added a comment. Nit: please add a release note and regenerate the ClangFormatStyleOptions.rst (if there are any changes because you modified Format.h). Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326

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

tnorth marked 7 inline comments as done. tnorth added a comment. > 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

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

tnorth updated this revision to Diff 240888. tnorth added a comment. - Add a unit-test loading a format and test file from temporary files. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72326/new/ https://reviews.llvm.org/D72326 Files:

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

tnorth updated this revision to Diff 238513. tnorth added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Thanks for your time and feedback. - Update diff with context, - remove public declaration of LoadConfigFile, - change prototype of LoadConfigFile and