[PATCH] D28315: Update tools to use new getStyle API

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D28315#641032, @amaiorano wrote:

> In https://reviews.llvm.org/D28315#639559, @ioeric wrote:
>
> > I ran `ninja check-all` with https://reviews.llvm.org/D28081 and https://reviews.llvm.org/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.


https://reviews.llvm.org/D28315