Update tools to use new getStyle API

[amaiorano]

Summary

See https://reviews.llvm.org/D28081 for the changes to getStyle

This change will be committed right after D28081.

[amaiorano]

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?

[malcolm.parsons]

A mailing list cannot review.

[hokein]

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.

[amaiorano]

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.

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

[hokein]

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.

[amaiorano]

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.

Yes, thanks for that. Unfortunately, the tests don't all pass when run using Visual Studio. I installed the requisite GNU utils, but get 25 failures. Will investigate a bit more, otherwise will use Linux.

Edit: to clarify, the tests fail without my changes.

[amaiorano]

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

FYI, tests were originally failing for me (without my changes) because I use Git, and it's configured to convert line endings to Windows (crlf) on checkout, but some of the tools tests specify extract offsets into source files, which end up being incorrect if you've got an extra byte per line ;) I'll open a ticket/discussion about it separately.

So if you give me the go ahead, I'll submit this change _after_ submitting D28081.

[ioeric]

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!

change-namespace/ChangeNamespace.cpp

889

I'd still like to apply replacements that are not cleaned up. Could you add the following line before continue, thanks! :)

FileToReplacements[FilePath] = Replaces;

[amaiorano]

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!

Will definitely double check. Strange that I didn't get these errors on Windows. I did notice in the output that it said one test was disabled. I'll take a closer look asap.

change-namespace/ChangeNamespace.cpp

889

Will do.

[amaiorano]

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.

[amaiorano]

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.

Okay, I figured it out. I needed to set CMake vars DLLVM_ENABLE_ASSERTIONS=ON and DLLVM_ABI_BREAKING_CHECKS=WITH_ASSERTS so that LLVM_ENABLE_ABI_BREAKING_CHECKS would be defined to 1, thus enabling the Expected<T> error you mentioned. I am now getting these same assertions. I will fix these and update the patch.

[amaiorano]

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.

[amaiorano]

change-namespace/ChangeNamespace.cpp

889

Ah darn, just realized that I forgot to make this change you asked for @ioeric. Let me know if everything else is okay, and I'll be sure to add this in before submitting.

[amaiorano]

change-namespace/ChangeNamespace.cpp

889

@ioeric Was looking at this code, and I'm wondering about the change you wanted me to make here. You said that you'd still like to apply replacements that are not cleaned up, so shouldn't that also be the case for when format::cleanupAroundReplacements fails (just below)? In other words, shouldn't it be:

  // Clean up old namespaces if there is nothing in it after moving.
  auto CleanReplacements =
      format::cleanupAroundReplacements(Code, Replaces, *Style);
  if (!CleanReplacements) {
    llvm::errs() << llvm::toString(CleanReplacements.takeError()) << "\n";
    FileToReplacements[FilePath] = Replaces;
    continue;
  }
  FileToReplacements[FilePath] = *CleanReplacements;
}

If so, I propose instead that we add

FileToReplacements[FilePath] = Replaces;

before the call to format::getStyle so that if either of the format-related calls below it (getStyle or cleanupAroundReplacements) fail, we get the unclean replacements.

[ioeric]

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.

change-namespace/ChangeNamespace.cpp

889

After taking a closer look, I realize that we don't actually need this change since Replaces is a reference to FileToReplacements[FilePath].

[amaiorano]

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

All tests pass (on Windows).

[amaiorano]

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.

[ioeric]

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?

[amaiorano]

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?

Thank you for testing it on Linux. No, there is no dependency on D28419 insofar as getting these 2 patches in. D28419 is about making sure tests work on Windows if you use Git and set core.autocrlf; however, I suspect the Windows build machines are using svn.

[ioeric]

Lgtm. Thanks!