Page MenuHomePhabricator

Update tools to use new getStyle API
ClosedPublic

Authored by amaiorano on Jan 4 2017, 1:45 PM.

Diff Detail

Event Timeline

amaiorano updated this revision to Diff 83121.Jan 4 2017, 1:45 PM
amaiorano retitled this revision from to Update tools to use new getStyle API.
amaiorano updated this object.

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?

A mailing list cannot review.

hokein added a subscriber: hokein.Jan 5 2017, 1:45 AM

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.

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 added a comment.Jan 5 2017, 7:45 AM

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 added a comment.EditedJan 6 2017, 8:41 AM

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.

@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 edited edge metadata.Jan 9 2017, 1:38 AM

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
892

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

FileToReplacements[FilePath] = Replaces;

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
892

Will do.

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.

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 updated this revision to Diff 83952.Jan 11 2017, 4:01 AM
amaiorano edited edge metadata.

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 added inline comments.Jan 11 2017, 5:31 AM
change-namespace/ChangeNamespace.cpp
892

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 added inline comments.Jan 15 2017, 1:51 PM
change-namespace/ChangeNamespace.cpp
892

@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.

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
892

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

amaiorano updated this revision to Diff 84539.Jan 16 2017, 3:36 AM

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

All tests pass (on Windows).

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.

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?

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 accepted this revision.Jan 16 2017, 5:16 AM
ioeric edited edge metadata.

Lgtm. Thanks!

This revision is now accepted and ready to land.Jan 16 2017, 5:16 AM
This revision was automatically updated to reflect the committed changes.