malaperle added inline comments.
================
Comment at: clangd/ClangdUnit.cpp:1117
+
+ if (!DeclVector.empty()) {
+ const Decl *LocationDecl = DeclVector[0];
----------------
malaperle wrote:
> malaperle wrote:
> > malaperle wrote:
> > > I think we need to simplify this part a bit. I'll try to find a way. Feel
> > > free to wait until more comments before updating.
> > Here's the version in which I tried to simplify this *a bit*. With the new
> > ErrorOr checks as well.
> >
> > ```
> > Hover clangd::getHover(ParsedAST &AST, Position Pos, clangd::Logger
> > &Logger) {
> > const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
> > const LangOptions &LangOpts = AST.getASTContext().getLangOpts();
> > const FileEntry *FE =
> > SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
> > if (!FE)
> > return Hover();
> >
> > SourceLocation SourceLocationBeg = getBeginningOfIdentifier(AST, Pos, FE);
> >
> > auto DeclMacrosFinder = std::make_shared<DeclarationAndMacrosFinder>(
> > llvm::errs(), SourceLocationBeg, AST.getASTContext(),
> > AST.getPreprocessor());
> > index::IndexingOptions IndexOpts;
> > IndexOpts.SystemSymbolFilter =
> > index::IndexingOptions::SystemSymbolFilterKind::All;
> > IndexOpts.IndexFunctionLocals = true;
> > indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
> > DeclMacrosFinder, IndexOpts);
> >
> > auto Decls = DeclMacrosFinder->takeDecls();
> > if (!Decls.empty() && Decls[0]) {
> > const Decl *LocationDecl = Decls[0];
> > std::vector<MarkedString> HoverContents;
> >
> > // Compute scope as the first "section" of the hover.
> > if (const NamedDecl *NamedD = dyn_cast<NamedDecl>(LocationDecl)) {
> > std::string Scope = getScope(NamedD,
> > AST.getASTContext().getLangOpts());
> > if (!Scope.empty()) {
> > MarkedString Info = {"", "C++", "In " + Scope};
> > HoverContents.push_back(Info);
> > }
> > }
> >
> > // Adjust beginning of hover text depending on the presence of
> > templates and comments.
> > TemplateDecl * TDec = LocationDecl->getDescribedTemplate();
> > const Decl * BeginDecl = TDec ? TDec : LocationDecl;
> > SourceLocation BeginLocation = BeginDecl->getSourceRange().getBegin();
> > RawComment *RC = AST.getASTContext().getRawCommentForDeclNoCache(
> > BeginDecl);
> > if (RC)
> > BeginLocation = RC->getLocStart();
> >
> > // Adjust end of hover text for things that have braces/bodies. We don't
> > // want to show the whole definition of a function, class, etc.
> > SourceLocation EndLocation = LocationDecl->getSourceRange().getEnd();
> > if (auto FuncDecl = dyn_cast<FunctionDecl>(LocationDecl)) {
> > if (FuncDecl->isThisDeclarationADefinition() && FuncDecl->getBody())
> > EndLocation = FuncDecl->getBody()->getLocStart();
> > } else if (auto TagDeclaration = dyn_cast<TagDecl>(LocationDecl)) {
> > if (TagDeclaration->isThisDeclarationADefinition())
> > EndLocation = TagDeclaration->getBraceRange().getBegin();
> > } else if (auto NameDecl = dyn_cast<NamespaceDecl>(LocationDecl)) {
> > SourceLocation BeforeLBraceLoc = Lexer::getLocForEndOfToken(
> > LocationDecl->getLocation(), 0, SourceMgr, LangOpts);
> > if (BeforeLBraceLoc.isValid())
> > EndLocation = BeforeLBraceLoc;
> > }
> >
> > SourceRange SR(BeginLocation, EndLocation);
> > if (SR.isValid()) {
> > auto L = getDeclarationLocation(AST, SR);
> > if (L) {
> > auto Ref = getBufferDataFromSourceRange(AST, *L);
> > if (Ref) {
> > Hover H;
> > if (SourceMgr.isInMainFile(BeginLocation))
> > H.range = L->range;
> > MarkedString MS = {"", "C++", *Ref};
> > HoverContents.push_back(MS);
> > H.contents = std::move(HoverContents);
> > return H;
> > }
> > }
> > }
> > }
> >
> > auto MacroInfos = DeclMacrosFinder->takeMacroInfos();
> > if (!MacroInfos.empty() && MacroInfos[0]) {
> > const MacroInfo *MacroInf = MacroInfos[0];
> > SourceRange SR(MacroInf->getDefinitionLoc(),
> > MacroInf->getDefinitionEndLoc());
> > if (SR.isValid()) {
> > auto L = getDeclarationLocation(AST, SR);
> > if (L) {
> > auto Ref = getBufferDataFromSourceRange(AST, *L);
> > if (Ref) {
> > Hover H;
> > if (SourceMgr.isInMainFile(SR.getBegin()))
> > H.range = L->range;
> > MarkedString MS = {"", "C++", "#define " + Ref->str()};
> > H.contents.push_back(MS);
> > return H;
> > }
> > }
> > }
> > }
> >
> > return Hover();
> > }
> > ```
> Here's the version in which I tried to simplify this *a bit*. With the new
> ErrorOr checks as well.
>
> ```
> Hover clangd::getHover(ParsedAST &AST, Position Pos, clangd::Logger &Logger) {
> const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
> const LangOptions &LangOpts = AST.getASTContext().getLangOpts();
> const FileEntry *FE =
> SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
> if (!FE)
> return Hover();
>
> SourceLocation SourceLocationBeg = getBeginningOfIdentifier(AST, Pos, FE);
>
> auto DeclMacrosFinder = std::make_shared<DeclarationAndMacrosFinder>(
> llvm::errs(), SourceLocationBeg, AST.getASTContext(),
> AST.getPreprocessor());
> index::IndexingOptions IndexOpts;
> IndexOpts.SystemSymbolFilter =
> index::IndexingOptions::SystemSymbolFilterKind::All;
> IndexOpts.IndexFunctionLocals = true;
> indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
> DeclMacrosFinder, IndexOpts);
>
> auto Decls = DeclMacrosFinder->takeDecls();
> if (!Decls.empty() && Decls[0]) {
> const Decl *LocationDecl = Decls[0];
> std::vector<MarkedString> HoverContents;
>
> // Compute scope as the first "section" of the hover.
> if (const NamedDecl *NamedD = dyn_cast<NamedDecl>(LocationDecl)) {
> std::string Scope = getScope(NamedD, AST.getASTContext().getLangOpts());
> if (!Scope.empty()) {
> MarkedString Info = {"", "C++", "In " + Scope};
> HoverContents.push_back(Info);
> }
> }
>
> // Adjust beginning of hover text depending on the presence of templates
> and comments.
> TemplateDecl * TDec = LocationDecl->getDescribedTemplate();
> const Decl * BeginDecl = TDec ? TDec : LocationDecl;
> SourceLocation BeginLocation = BeginDecl->getSourceRange().getBegin();
> RawComment *RC = AST.getASTContext().getRawCommentForDeclNoCache(
> BeginDecl);
> if (RC)
> BeginLocation = RC->getLocStart();
>
> // Adjust end of hover text for things that have braces/bodies. We don't
> // want to show the whole definition of a function, class, etc.
> SourceLocation EndLocation = LocationDecl->getSourceRange().getEnd();
> if (auto FuncDecl = dyn_cast<FunctionDecl>(LocationDecl)) {
> if (FuncDecl->isThisDeclarationADefinition() && FuncDecl->getBody())
> EndLocation = FuncDecl->getBody()->getLocStart();
> } else if (auto TagDeclaration = dyn_cast<TagDecl>(LocationDecl)) {
> if (TagDeclaration->isThisDeclarationADefinition())
> EndLocation = TagDeclaration->getBraceRange().getBegin();
> } else if (auto NameDecl = dyn_cast<NamespaceDecl>(LocationDecl)) {
> SourceLocation BeforeLBraceLoc = Lexer::getLocForEndOfToken(
> LocationDecl->getLocation(), 0, SourceMgr, LangOpts);
> if (BeforeLBraceLoc.isValid())
> EndLocation = BeforeLBraceLoc;
> }
>
> SourceRange SR(BeginLocation, EndLocation);
> if (SR.isValid()) {
> auto L = getDeclarationLocation(AST, SR);
> if (L) {
> auto Ref = getBufferDataFromSourceRange(AST, *L);
> if (Ref) {
> Hover H;
> if (SourceMgr.isInMainFile(BeginLocation))
> H.range = L->range;
> MarkedString MS = {"", "C++", *Ref};
> HoverContents.push_back(MS);
> H.contents = std::move(HoverContents);
> return H;
> }
> }
> }
> }
>
> auto MacroInfos = DeclMacrosFinder->takeMacroInfos();
> if (!MacroInfos.empty() && MacroInfos[0]) {
> const MacroInfo *MacroInf = MacroInfos[0];
> SourceRange SR(MacroInf->getDefinitionLoc(),
> MacroInf->getDefinitionEndLoc());
> if (SR.isValid()) {
> auto L = getDeclarationLocation(AST, SR);
> if (L) {
> auto Ref = getBufferDataFromSourceRange(AST, *L);
> if (Ref) {
> Hover H;
> if (SourceMgr.isInMainFile(SR.getBegin()))
> H.range = L->range;
> MarkedString MS = {"", "C++", "#define " + Ref->str()};
> H.contents.push_back(MS);
> return H;
> }
> }
> }
> }
>
> return Hover();
> }
> ```
```
if (SourceMgr.isInMainFile(SR.getBegin()))
H.range = L->range;
```
This is not quite right in case you have a macro definition in the preamble but
still in the same "document" (they are different FileIDs for the
SourceManager!). For example:
```
#include "foo.h"
#define MY_MACRO 1
void bar() {
MY_MACRO;
}
```
The #define is *also* part of the preamble so isMainFile will return false. So
in that case, I'm not sure what's the best way to make sure the macro expansion
and the definition are in the same file but comparing FileEntry should work
although a bit verbose:
```
FileID FID =
SourceMgr.getFileID(SourceMgr.getExpansionLoc(SR.getBegin()));
bool IsInMainFile = FID.isValid() && SourceMgr.getMainFileID() == FID;
if (!IsInMainFile) {
// Special case when a macro is defined in a preamble, it could
still be in the "main file", below the inclusions.
// The SourceManager considers the preamble and the main file as
two different FileIDs but the FileEntries should match.
bool IsInPreamble = FID == SourceMgr.getPreambleFileID();
if (IsInPreamble)
IsInMainFile =
SourceMgr.getFileEntryForID(SourceMgr.getMainFileID()) ==
SourceMgr.getFileEntryForID(FID);
}
if (IsInMainFile)
H.range = L->range;
```
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D35894
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
