[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#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.


https://reviews.llvm.org/D28315