See https://reviews.llvm.org/D28081 for the changes to getStyle
This change will be committed right after D28081.
See https://reviews.llvm.org/D28081 for the changes to getStyle
This change will be committed right after D28081.
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?
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.
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; |
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. |
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.cppError 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.
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.
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. |
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]. |
Rebased changes on latest. No functional changes in this diff since last.
All tests pass (on Windows).
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?