ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
Another round of review
================
Comment at: clangd/ClangdUnit.cpp:113
+ CppFilePreambleCallbacks(IncludeReferenceMap &IRM)
+ : IRM(IRM) {}
----------------
ilya-biryukov wrote:
> Let's create a new empty map inside this class and have a
> `takeIncludeReferences` method (similar to `TopLevelDeclIDs` and
> `takeTopLevelDeclIDs`)
This comment is not addressed yet, but marked as done.
================
Comment at: clangd/ClangdUnit.cpp:147
+ IncludeReferenceMap &IRM;
+ std::vector<std::pair<SourceRange, std::string>>
TempPreambleIncludeLocations;
};
----------------
Nebiroth wrote:
> ilya-biryukov wrote:
> > We should have `SourceMgr` at all the proper places now, let's store
> > `IncludeReferenceMap` directly
> The idea was to pass a single map around to fill with every reference instead
> of having separate maps being merged together.
You copy the map for preamble and then append to it in
`CppFilePreambleCallbacks`? That also LG, we should not have many references
there anyway.
================
Comment at: clangd/ClangdUnit.cpp:281
+ IntrusiveRefCntPtr<vfs::FileSystem> VFS,
+ IncludeReferenceMap IRM) {
----------------
Nebiroth wrote:
> ilya-biryukov wrote:
> > What reference does this `IncludeReferenceMap` contain? Includes from the
> > preamble?
> This should contain every reference available for one file.
Thanks for clarifying, LG.
================
Comment at: clangd/ClangdUnit.cpp:141
+ std::unique_ptr<PPCallbacks> createPPCallbacks() {
+ return llvm::make_unique<IncludeRefsCollector>(*SourceMgr, IRM);
+ }
----------------
Maybe add `assert(SourceMgr && "SourceMgr must be set at this point")` here?
================
Comment at: clangd/ClangdUnit.cpp:281
+ std::shared_ptr<PCHContainerOperations> PCHs,
+ IntrusiveRefCntPtr<vfs::FileSystem> VFS, IncludeReferenceMap IRM) {
----------------
Don't we already store the map we need in `PreambleData`? Why do we need an
extra `IRM` parameter?
================
Comment at: clangd/ClangdUnit.h:276
+std::vector<Location>
+findDefinitions(const Context &Ctx, ParsedAST &AST, Position Pos);
+
----------------
ilya-biryukov wrote:
> This function moved without need and lost a comment.
Now we have duplicated definitions here. Bad merge?
================
Comment at: clangd/CodeComplete.cpp:328
unsigned NumResults) override final {
- if (auto SS = Context.getCXXScopeSpecifier())
- CompletedName.SSInfo = extraCompletionScope(S, **SS);
+// if (auto SS = Context.getCXXScopeSpecifier())
+// CompletedName.SSInfo = extraCompletionScope(S, **SS);
----------------
Accidental change?
================
Comment at: clangd/CodeComplete.cpp:807
// Enable index-based code completion when Index is provided.
- Result.IncludeNamespaceLevelDecls = !Index;
+ // Result.IncludeNamespaceLevelDecls = !Index;
----------------
Accidental change?
================
Comment at: clangd/XRefs.cpp:41
+ std::vector<Location> takeLocations() {
+ // Don't keep the same location multiple times.
----------------
We don't need locations anymore. Remove them.
================
Comment at: clangd/XRefs.cpp:177
+ std::vector<Location> IRMResult;
+ if (!AST.getIRM().empty()) {
----------------
Probably a good place for a comment. Also, maybe rename local var to something
easier to understand like `IncludeDefinitions`
```
/// Process targets for paths inside #include directive.
std::vector<Location> IncludeTargets;
```
================
Comment at: clangd/XRefs.cpp:178
+ std::vector<Location> IRMResult;
+ if (!AST.getIRM().empty()) {
+ for (auto &IncludeLoc : AST.getIRM()) {
----------------
No need to special case empty maps, remove the if altogether.
================
Comment at: clangd/XRefs.cpp:183
+ unsigned CharNumber = SourceMgr.getSpellingColumnNumber(
+ DeclMacrosFinder->getSearchedLocation());
+
----------------
Replace with `DeclMacrosFinder->getSearchedLocation()` `SourceLocationBeg`, it
makes the code easier to read.
================
Comment at: clangd/XRefs.cpp:185
+
+ if ((unsigned)R.start.line ==
+ SourceMgr.getSpellingLineNumber(
----------------
why do we need to convert to unsigned? To slience compiler warnings?
================
Comment at: clangd/XRefs.cpp:188
+ DeclMacrosFinder->getSearchedLocation()) &&
+ ((unsigned)R.start.character >= CharNumber &&
+ CharNumber <= (unsigned)R.end.character)) {
----------------
this should be `R.start.charater <= CharNumber && CharNumber <= R.end.character`
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
