[PATCH] D150185: [include-cleaner] Allow multiple strategies for spelling includes.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks, a few comments to simplify the code/test further.



================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:94
+/// shortest spelling.
+std::string spellHeader(const IncludeSpellerInput &Input);
 } // namespace include_cleaner
----------------
can you move it to the `IncludeSpeller.h` file? I think it is a more suitable place.




================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/IncludeSpeller.h:22
+/// Provides the necessary information for custom spelling computations.
+struct IncludeSpellerInput {
+  const Header &H;
----------------
This struct is the input for the IncludeSpeller interface, thus I prefer to move the structure to IncludeSpeller class, and call it `Input`. 


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:83
 
-std::string spellHeader(const Header &H, HeaderSearch &HS,
-                        const FileEntry *Main) {
+std::string spellHeader(const IncludeSpellerInput &Input) {
+  const Header &H = Input.H;
----------------
similar to .h file, I think we should move this implementation to the `IncludeSpeller.cpp` file.


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:96
+
+    // Fallback to default spelling via header search.
+    bool IsSystem = false;
----------------
I think it may be clearer to make this fallback spelling implementation as a `DefaultIncludeSpeller` class, and append it to the end of `Spellers` (but not register it to make sure it always the last one of the spelling IncludeSpellingStrategy). 

The code looks like 

```
class DefaultIncludeSpeller : IncludeSpeller {
   ...
};
std::string mapHeader(const IncludeSpellerInput &Input) {
  static auto Spellers = [] {
    auto Result =
        llvm::SmallVector<std::unique_ptr<include_cleaner::IncludeSpeller>>{};
    for (const auto &Strategy :
         include_cleaner::IncludeSpellingStrategy::entries())
      Result.push_back(Strategy.instantiate());
    Result.push_back(std::make_unique<DefaultIncludeSpeller>());
    return Result;
  }();
  ....
}

std::string spellerHeader(...) {
   ...
   case Header::Physical:
      return mapHeader(Input);
}
```


================
Comment at: clang-tools-extra/include-cleaner/unittests/IncludeSpellerTest.cpp:2
+//===--- IncludeSpellerTest.cpp
+//-------------------------------------------------===//
+//
----------------
line: line 1 and line 2 should be merged into a single line.


================
Comment at: clang-tools-extra/include-cleaner/unittests/IncludeSpellerTest.cpp:60
+        AbsolutePath.consume_front(llvm::StringRef{RootWithSeparator});
+    if (!Result)
+      return "";
----------------
nit: inline the Result.

```
if (!AbsolutePath.consume_front())
  return "";
return "\"" + AbsolutePath.str() + "\"";
```


================
Comment at: clang-tools-extra/include-cleaner/unittests/IncludeSpellerTest.cpp:81
+
+  EXPECT_EQ("foo.h", spellHeader({Header{*FM.getFile(testPath("foo.h"))}, HS,
+                                  MainFile}));
----------------
I think there should be `""` around the `foo.h`, the `spellHeader` API returns the spelling contains the ""<> characters, so we need to add "" to the result in the `DummyIncludeSpeller::operator()` method.


================
Comment at: clang-tools-extra/include-cleaner/unittests/IncludeSpellerTest.cpp:90
+                   HS, MainFile}));
+  EXPECT_EQ("\"foo/bar.h\"",
+            spellHeader({Header{*FM.getFile("foo/bar.h")}, HS, MainFile}));
----------------
I think the unittest is mainly for testing our llvm-registry mechanism work by verifying DummyIncludeSpeller is used for file path starting with `clangd-test`, and fallback one is used otherwise. 

we're less interested in the the different file separator per platform. Let's try to simplify the unittest to avoid subtle platform issues, something like below should cover the thing we want:

```
Inputs.ExtraFiles["/include-cleaner-test/foo.h"] = "";
Inputs.ExtraFiles["system_header.h"] = "";

Inputs.ExtraFlags = {"-isystem."};
...

EXPECT_EQ("\"foo.h\"",
            spellHeader({Header{*FM.getFile("foo.h")}, HS, MainFile})); // spelled by the dummerIncludeSpeller.
EXPECT_EQ("<system_header.h>",
            spellHeader({Header{*FM.getFile("system_header.h")}, HS, MainFile})); // spelled by the fallback one.
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150185/new/

https://reviews.llvm.org/D150185