[PATCH] D35194: [clang-tidy] clang-apply-replacements: Don't insert null entry

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

Thank you for the fix! A few comments below.



================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:291-299
       const FileEntry *Entry = SM.getFileManager().getFile(R.getFilePath());
-      if (!Entry && Warned.insert(R.getFilePath()).second) {
-        errs() << "Described file '" << R.getFilePath()
-               << "' doesn't exist. Ignoring...\n";
+      if (!Entry) {
+        if (Warned.insert(R.getFilePath()).second) {
+          errs() << "Described file '" << R.getFilePath()
+                 << "' doesn't exist. Ignoring...\n";
+        }
         continue;
----------------
I'd swap the positive and the negative branches and remove `continue`:

  if (const FileEntry *Entry = ...) {
    GroupedReplacements[Entry].push_back(R);
  } else if (...) {
    errs() << ...;
  }

Same below.


================
Comment at: test/clang-apply-replacements/invalid-files.cpp:1
+// RUN: mkdir -p %T/Inputs/invalid-files
+// RUN: clang-apply-replacements %T/Inputs/invalid-files
----------------
No need for `Inputs` in the path inside the build directory.


================
Comment at: test/clang-apply-replacements/invalid-files.cpp:5-7
+// RUN: ls -1 %T/Inputs/invalid-files | FileCheck %s --check-prefix=YAML
+//
+//// YAML: {{.+\.yaml$}}
----------------
FileCheck is not needed here. Just `// RUN: ls %T/invalid-files/invalid-files.yaml` should be enough to assert the file existence.


https://reviews.llvm.org/D35194