In D29221#713902, @hans wrote:

In D29221#698679, @amaiorano wrote:

Hey guys, any feedback on this? About 1.5 weeks ago, @hans asked @klimek and @djasper for their opinions on "clang-format" vs "ClangFormat". Thanks!

Let's get this committed now and not worry about renaming things.

Do you have commit access, or would you like me to commit it for you?

In D29221#688697, @hans wrote:

My only nit is that I'd prefer "clang-format" instead of "ClangFormat".

Manuel: the menu options under Tools currently say "Clang Format {Selection,Document}". What do you think about using "clang-format" here instead? That is the name of the tool after all, and I think it also works nicely as a verb.

I realize the actual extension is called ClangFormat, but maybe there were reasons for that, or we have to live with it?

I would also like clang-format in there rather than ClangFormat. One thing to validate is whether this change would mean people would lose their changes to the defaults in the configuration panel. I'll run some tests and see if this is indeed the case. Maybe there's a way to keep the internal name the same for config purposes, but change the displayed name instead. Will get back to you.

Okay, I looked into it, and the good news is that changing the name of the category from ClangFormat to clang-format doesn't reset the previously saved changes made in the options menu. I also changed the menu item names. Here's what both would look like:

Now having said that, I'm not sure if this is the best change because of a few things:

1. The extension's name itself is ClangFormat, which was recently added to the Visual Studio market place here: https://marketplace.visualstudio.com/items?itemName=HansWennborg.ClangFormat . Consequently, the extension appears with this name in the Visual Studio extensions dialog:
   
   . I don't think it would be easy to change this. You cannot really take down an extension page on the marketplace once it's there; you'd have to document that it has been deprecated and provide a link to a new page. When searching for it in the Extensions dialog in VS, though, both the old and new extension would show up.
2. Although the name of the executable is indeed "clang-format", the documentation here uses the name ClangFormat:
   
   
   
   So I leave it up to you whether you really want this change or not. We can also decide later rather than fold it into this change.

Personally I think we should refer to the tool and the action of formatting as "clang-format" as much as possible. It's unfortunate we can't rename the extension, but maybe that slight inconsistency isn't the end of the world.

Manuel, Daniel: I'd like to hear your opinions here.

In D29221#687438, @amaiorano wrote:

In D29221#687425, @hans wrote:

In D29221#686865, @amaiorano wrote:

Made changes based on @hans 's feedback.

I looked again at the categories, and I think it makes sense with my changes. Here's an updated screenshot that shows what the options menu in Visual Studio looks like with these changes with the top-level category name ("LLVM/Clang") and page name ("Clang Format") highlighted, as well as the category names for the page items ("Format Options" and "Format On Save") highlighted:

It would be nice if "Format On Save" was _below_ "Format Options", but Visual Studio always orders the page categories in alphabetical order, so this is pretty standard.

The screenshot looks good.

My only nit is that I'd prefer "clang-format" instead of "ClangFormat".

Manuel: the menu options under Tools currently say "Clang Format {Selection,Document}". What do you think about using "clang-format" here instead? That is the name of the tool after all, and I think it also works nicely as a verb.

I realize the actual extension is called ClangFormat, but maybe there were reasons for that, or we have to live with it?

I would also like clang-format in there rather than ClangFormat. One thing to validate is whether this change would mean people would lose their changes to the defaults in the configuration panel. I'll run some tests and see if this is indeed the case. Maybe there's a way to keep the internal name the same for config purposes, but change the displayed name instead. Will get back to you.

In D29221#687425, @hans wrote:

In D29221#686865, @amaiorano wrote:

Made changes based on @hans 's feedback.

I looked again at the categories, and I think it makes sense with my changes. Here's an updated screenshot that shows what the options menu in Visual Studio looks like with these changes with the top-level category name ("LLVM/Clang") and page name ("Clang Format") highlighted, as well as the category names for the page items ("Format Options" and "Format On Save") highlighted:

It would be nice if "Format On Save" was _below_ "Format Options", but Visual Studio always orders the page categories in alphabetical order, so this is pretty standard.

The screenshot looks good.

My only nit is that I'd prefer "clang-format" instead of "ClangFormat".

Manuel: the menu options under Tools currently say "Clang Format {Selection,Document}". What do you think about using "clang-format" here instead? That is the name of the tool after all, and I think it also works nicely as a verb.

I realize the actual extension is called ClangFormat, but maybe there were reasons for that, or we have to live with it?

Made changes based on @hans 's feedback.

Hello again. Just wondering if anyone could take a look at this? I updated the patch a week ago :) Thanks!

• Renamed VsixUtils to Vsix and TypeConverterUtils to TypeConversion
• Removed format on save mode

In D29221#668867, @klimek wrote:

+hans

+1 to "format only current document but save all" not making much sense :)

Hello! Just a small ping to see if anyone has taken a look at this change? I fully understand that everyone's really busy, but perhaps you can recommend another reviewer? Or if you're presently giving this change a whirl, just let me know! Cheers :)

This change is no longer needed since clang-format now returns failure status (non-zero) whenever it writes to stderr (e.g. on parse error) since https://llvm.org/svn/llvm-project/cfe/trunk@292174 (D28081)

In D28943#651489, @ioeric wrote:

In D28943#651488, @amaiorano wrote:

In D28943#651470, @ioeric wrote:

@amaiorano: The test itself is correct. It's just that this test failed in our internal test. We could've fixed it internally, but the fix would be ugly. Since the intended behavior is already covered in the case above it, and it's really just checking the default fallback style is LLVM, which is not related to the original change, I think it makes sense to get rid of the case. Hope you don't mind :)

Of course I don't mind :) Why did it fail your internal tests, btw? Just curious. Was it something I could've detected myself?

Probably not... it's just that our default fallback style is "Google" instead of "LLVM".

In D28943#651470, @ioeric wrote:

@amaiorano: The test itself is correct. It's just that this test failed in our internal test. We could've fixed it internally, but the fix would be ugly. Since the intended behavior is already covered in the case above it, and it's really just checking the default fallback style is LLVM, which is not related to the original change, I think it makes sense to get rid of the case. Hope you don't mind :)

In D28315#647212, @ioeric wrote:

In D28315#647154, @amaiorano wrote:

In D28315#647103, @ioeric wrote:

Let me know when broken tests are fixed and this patch (and the corresponding patch) is ready again for review. Also let me know if you need any help.

I updated the two patches after rebasing on latest (no conflicts). These should be ready to go. If you don't mind testing the two again on Linux, that would be greatly appreciated. Otherwise, on Windows the tests pass.

All tests passed on Linux. Are these patches still depending on D28419? Or the previous issue has been somehow resolved on Windows?

In D28315#647103, @ioeric wrote:

Let me know when broken tests are fixed and this patch (and the corresponding patch) is ready again for review. Also let me know if you need any help.

Rebased changes on latest. No functional changes in this diff since last.

Rebased changes on latest, no functional change in this diff.

Fixed code in ClangApplyReplacementsMain.cpp so that we only handle the Expected<Style> when DoFormat is true. Note that I'm using the unidiomatic FormatStyleOrError variable name here, but that's because we only want to set the FormatStyle variable in the parent scope when DoFormat is true.

Fixed Format/style-on-command-line.cpp test to match new expected output.

Last night, I realized that this problem extends to the clang/tests as well, not just those in clang-tools-extra. Is there someone who knows Windows, Git, and the testing pipeline that can weigh in more on this topic?

In D28315#641032, @amaiorano wrote:

In D28315#639559, @ioeric wrote:

I ran ninja check-all with D28081 and D28315 (in Linux), and some lit tests failed, namely:

Clang :: Format/style-on-command-line.cpp

This was the test that is explicitly disabled on Windows. I enabled it locally and modified the test to match the modified output and expected non-zero result (patch forth-coming once I understand what's up with the failed tests you mention below...)

Clang Tools :: clang-apply-replacements/basic.cpp
Clang Tools :: clang-apply-replacements/conflict.cpp
Clang Tools :: clang-apply-replacements/crlf.cpp
Clang Tools :: clang-apply-replacements/format.cpp
Clang Tools :: clang-rename/ClassReplacements.cpp

Error message I am seeing: `Expected<T> must be checked before access or destruction.`

Could you double check? Thanks!

These tests pass for me. For example, when I run llvm-lit explicitly on clang-apply-replacements/basic.cpp, I get:

C:\code\llvm-build-msvc\Debug\bin>llvm-lit.py -a -v c:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements\basic.cpp
-- Testing: 1 tests, 1 threads --
PASS: Clang Tools :: clang-apply-replacements/basic.cpp (1 of 1)
Script:
--
mkdir -p C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
grep -Ev "// *[A-Z-]+:" C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/basic.h > C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h
sed "s#\$(path)#C:/code/llvm-build-msvc/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#" C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/file1.yaml > C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/file1.yaml
sed "s#\$(path)#C:/code/llvm-build-msvc/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#" C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/file2.yaml > C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/file2.yaml
clang-apply-replacements C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
FileCheck -input-file=C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/basic.h
ls -1 C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic | FileCheck C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements\basic.cpp --check-prefix=YAML
grep -Ev "// *[A-Z-]+:" C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/basic.h > C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h
clang-apply-replacements -remove-change-desc-files C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
ls -1 C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic | FileCheck C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements\basic.cpp --check-prefix=NO_YAML
--
Exit Code: 0

Command Output (stdout):
--
$ "mkdir" "-p" "C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
$ "grep" "-Ev" "// *[A-Z-]+:" "C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/basic.h"
$ "sed" "s#\$(path)#C:/code/llvm-build-msvc/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#" "C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/file1.yaml"
$ "sed" "s#\$(path)#C:/code/llvm-build-msvc/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#" "C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/file2.yaml"
$ "clang-apply-replacements" "C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
$ "FileCheck" "-input-file=C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h" "C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/basic.h"
$ "ls" "-1" "C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
$ "FileCheck" "C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements\basic.cpp" "--check-prefix=YAML"
$ "grep" "-Ev" "// *[A-Z-]+:" "C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/basic.h"
$ "clang-apply-replacements" "-remove-change-desc-files" "C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
$ "ls" "-1" "C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
$ "FileCheck" "C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements\basic.cpp" "--check-prefix=NO_YAML"

--

********************
Testing Time: 0.22s
  Expected Passes    : 1

Would you mind running this same command on Linux (with -a -v) and seeing what's different? I don't see why this test would run differently on Windows.

Having said that, I believe I can see one place where I could get Expected<T> must be checked before access or destruction.: in ClangApplyReplacementsMain.cpp, I modified the code to:

llvm::Expected<format::FormatStyle> FormatStyle = format::getNoStyle();
if (DoFormat) {
  FormatStyle = format::getStyle(FormatStyleOpt, FormatStyleConfig, "LLVM");
  if (!FormatStyle) {
    llvm::errs() << llvm::toString(FormatStyle.takeError()) << "\n";
    return 1;
  }
}

If DoFormat is false, then FormatStyle will not be checked. I can fix this, but I would love to get this actual error out of the test myself.

In D28315#639559, @ioeric wrote:

I ran ninja check-all with D28081 and D28315 (in Linux), and some lit tests failed, namely:

Clang :: Format/style-on-command-line.cpp

In D28315#639559, @ioeric wrote:

I ran ninja check-all with D28081 and D28315 (in Linux), and some lit tests failed, namely:

Clang :: Format/style-on-command-line.cpp
Clang Tools :: clang-apply-replacements/basic.cpp
Clang Tools :: clang-apply-replacements/conflict.cpp
Clang Tools :: clang-apply-replacements/crlf.cpp
Clang Tools :: clang-apply-replacements/format.cpp
Clang Tools :: clang-rename/ClassReplacements.cpp

Error message I am seeing: Expected<T> must be checked before access or destruction.

Could you double check? Thanks!

Hello, sorry for the lack of details here. I will re-run the tests later and report here the tests that are failing along with output.

@ioeric Okay, check-clang-tools passes with my changes.

In D28315#636888, @hokein wrote:

In D28315#636821, @amaiorano wrote:

Is there a way to run tests without ninja? I'm on Windows. If not, I'll use my Linux VM.

It is not related to the build system. There should be a check-clang-tools project, you can run tests by building it. For example, if you are using make, just run make check-clang-tools; If you are using Visual Studio, build the check-clang-tools project.

In D28315#636670, @hokein wrote:

In D28315#635865, @amaiorano wrote:

I made these changes, and they build, but I didn't really test them. Are there unit tests for these tools that I can run?

If you use ninja build, you can run ninja check-clang-tools to run all tests.

@ioeric I think you removed cfe-commits as a reviewer, then added it back, but it didn't work. Should I re-add?

I made these changes, and they build, but I didn't really test them. Are there unit tests for these tools that I can run?

In D28081#633786, @ioeric wrote:

The patch LGTM now. I'll accept both this and the one for clang-tool-extra when it is ready.

Regarding builbots, we have bots that continually run builds/tests (http://lab.llvm.org:8011/). Many buildbots test llvm and clang as well as clang-tools-extra (e.g. with ninja check-all) at some revision. Also note that although llvm/clang and clang-tools-extra are different repos, they do share the same revision sequence. So if clang-tools-extra is in a inconsistent state, many buildbots can fail and affect llvm/clang builds. Unfortunately, there is no atomic way to commit two revisions to two repositories, so we just commit them quickly one after another so that we do less damage. Do you have commit access to LLVM btw?

Minor comment change, turned the ObjC test into a non-fixture test, and renamed FormatStyleOrError to FormatStyle in format function.

In D27440#634043, @cameron314 wrote:

Thanks, I'll check these out! Btw, I noticed that the clang-format tests are non-Windows due to path assumptions. Is this a lost cause, or just something no one's bothered to look into yet?

No one's bothered looking into it yet.

Which tests? We run the unit tests (FormatTests.exe) locally just fine on Windows.

More changes as suggested by @ioeric. I asked a question earlier about clang-tools-extras and understanding how not to break the build bots. If you can shed some light on that, I'd appreciate it, thanks :)

In D28081#633521, @ioeric wrote:

Some nits. Some is almost good :)

BTW, do you have clang-tools-extra in your source tree? There are also some references in the subtree to the changed interface. It would be nice if you could also fix them in a separate patch and commit these two patches together (I mean, within a short period of time) so that you wouldn't break build bots.

References should be found in these files:

extra/change-namespace/ChangeNamespace.cpp
extra/clang-move/ClangMove.cpp
extra/include-fixer/tool/ClangIncludeFixer.cpp	 
extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
extra/clang-tidy/ClangTidy.cpp

Thanks!

Reverted the FallbackStyle code and added a FIXME as @ioeric suggested. I'll fix the fallback style "none" bug in a separate change.

Hello everyone, so after a few more tests, I've uncovered a bug and perhaps a different meaning for fallback style. First, the bug: if you set fallback style to "none", clang-format will perform no replacements. This happens because getStyle will first initialize its local Style variable to LLVM style, and then because a fallback style is set, will then set it to the "none" style, will ends up setting Style.DisableFormatting to true. After that, when we parse YAML (either from Style arg or a config file), we use the Style variable as the "template" for fields that haven't been set. In this case, the "none" fallback style causes DisableFormatting to remain true, so no formatting will take place.

Updated with the changes @ioeric suggested.

Thank you, @ioeric for your feedback!

One more thing I forgot to mention is that this change comes from a discussion we had on this other change: https://reviews.llvm.org/D27440 where @djasper agreed that "fallback-style should only be used when there is no .clang-format file. If we find one, and it doesn't parse correctly, we should neither use the fallback style nor scan in higher-level directories (not sure whether we currently do that).".

This change works, and passes all tests; however, I have a bunch of questions, which you'll find in the diffs. I look forward to your feedback. Thanks!

As agreed, modified getStyle to make use of vfs::FileSystem::makeAbsolute. I would modify the body of the commit as follows:

In D27971#627543, @klimek wrote:

Why isn't the right solution to make getStyle() use vfs::FileSystem? Generally, everything in clang-format (well, in clang) should use vfs::FileSystem for file access.

Updated screenshot:

Added "na" format specifier to not show the pointer address to the string, just like the original visualizer did. Updated screenshot of what it looks like forthcoming...

Goes from:

In D27440#626415, @ioeric wrote:

llvm::ErrorOr carries std::error_code. If you want richer information (e.g. error_code + error message), llvm::Expcted<T> and llvm::Error are your friends.

FYI, if you only need error_code + error_message in the returned error, there is also llvm::StringError. And if you want to carry even more information in the errors, you can implement llvm::ErrorInfo, which is what we are doing in libTooling replacements library: https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Core/Replacement.h#L150

In D27440#624917, @djasper wrote:

Yes.. return non-zero seems right. This is an error condition.

In D27440#624773, @djasper wrote:

I agree that fallback-style should only be used when there is no .clang-format file. If we find one, and it doesn't parse correctly, we should neither use the fallback style nor scan in higher-level directories (not sure whether we currently do that).

Hi @djasper, just curious about your opinion on this change, and on the conversation following it, specifically regarding whether clang-format should really use the fallback style when failing to parse a .clang-format file. Thanks!

Hi @djasper, just curious about your opinion on this change, and on the conversation following it, specifically regarding whether clang-format should really use the fallback style when failing to parse a .clang-format file. Thanks!

(NOTE, I forgot to click Submit on the reply to @klimek yesterday)

My first patch accidentally included the changes from https://reviews.llvm.org/D27440

Yes, I think this is still a worthwhile change. Let's add the hour and minute and get it landed.

Added hour and minute.

In D27440#614337, @klimek wrote:

Pondering this a bit - one question is whether we should make clang-format not return 0 if we pass -fallback-style=none (it already has this option) and we can't figure out a style. What do you think?

In D27438#614219, @hans wrote:

When building the snapshots and releases, I usually put the SVN revision number in there to avoid this same problem: http://llvm-cs.pcc.me.uk/utils/release/build_llvm_package.bat#25

With this change, I now get the following message box when my .clang-format has an invalid field named "SomeInvalidField":