hokein added a comment.
Thanks, this looks great, some more comments (most are nits)
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1099
+ const SourceManager &SM = AST.getSourceManager();
+ llvm::SmallVector<include_cleaner::Header> Headers =
+ include_cleaner::headersForSymbol(Sym, SM, AST.getPragmaIncludes());
----------------
maybe name it `SortedProviders`, it contains the main-file (calling Headers is
a bit confusing).
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1111
+ // Main file ranked higher than any #include'd file
+ break;
+
----------------
if we perform an early `return` inside the loop, the code can be simpler.
```
// Pickup the highest provider that satisfied the symbol usage, if available.
// If the main-file is the chosen provider, we deliberately do not display it
(as it may cause too much noise, e.g. for local variables).
for (const auto & H : Headers) {
if (isMainFile(H)) {
return;
}
if (!Matches.empty()) {
HI.Provider = Matches[0]->quote();
return;
}
}
HI.Provider = spellHeader(AST...);
```
================
Comment at: clang-tools-extra/clangd/Hover.h:71
std::string Name;
+ /// Header providing the symbol (best match).
+ std::string Provider;
----------------
nit: mention this header is with `""<>`.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.h:71
+include_cleaner::Includes
+convertIncludes(const SourceManager &SM,
----------------
nit: can you add some document comment on these two functions?
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2896
+ struct Foo {};
+ Foo F = [[Fo^o]]{};
+)cpp"),
----------------
looks like `[[]]` is not used in the test, we only use the `^` point. Remove
them?
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2897
+ Foo F = [[Fo^o]]{};
+)cpp"),
+ [](HoverInfo &HI) { HI.Provider = ""; }},
----------------
I think making the indentation like below is cleaer:
```
{Annotations(R"cpp(
struct Foo {};
int F = [[FO^O]]{};
)cpp"),
[](HoverInfo &HI) { HI.Provider = ""; }},
```
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2911
+ #define FOO 5
+ int F = [[^FOO]];
+ )cpp"),
----------------
The indentation of the code block doesn't seem to align with the above cases.
Unfortunately, clang-format doesn't format the codeblock inside the `cpp()cpp`
string literal, we need to do it manually.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2938
+ )cpp"),
+ [](HoverInfo &HI) { HI.Provider = "\"foo.h\""; }}};
+
----------------
I think this should be `""`, the using declaration is defined in main-file,
main-file should be the only one provider, and we don't display main-file
provider.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:16
#include "clang-include-cleaner/Types.h"
+#include "clang/Basic/SourceLocation.h"
#include "clang/Format/Format.h"
----------------
we have been using forward declaration for the `SourceLocation`, we can add a
forward declaration `class SourceManager` on Line 26, and get rid of the
#include here.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:84
+/// likely provider for the symbol.
+llvm::SmallVector<Header> headersForSymbol(const Symbol &S,
+ const SourceManager &SM,
----------------
since we move it from `AnalysisInternal.h`, can you remove the one in
`AnalysisInternal.h`? I think we need to add `#include "Analysis.h"` to
`AnalysisInternal.h`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144976/new/
https://reviews.llvm.org/D144976
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
