[PATCH] D44247: [clangd] Use identifier range as the definition range.

Haojian Wu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Mar 9 03:38:18 PST 2018

hokein added a comment.

Thanks for the comments!

Comment at: clangd/SourceCode.cpp:55
+  const auto& SM = D->getASTContext().getSourceManager();
+  SourceLocation SpellingLoc = SM.getSpellingLoc(D->getLocation());
+  if (D->getLocation().isMacroID()) {
sammccall wrote:
> As discussed offline, this heuristic is... limited.
> ```#define X Y
> class X{}```
> and now we say the definition of the class Y is on the first line.
> I think we should really be basing the heuristics on the whole definition range, even if we're then going to return just the position of the name.
> This patch just moves logic around so we don't need to do it now, but it could use a FIXME
Thanks for pointing it out! Added a FIXME. 

Comment at: clangd/SourceCode.h:34
+/// Get the source location of the given D.
sammccall wrote:
> This is a useful function, but it doesn't belong in SourceCode with its current scope. See the file header :)
> We could add an "AST.h" or similar file, or expand the scope of SourceCode, but we haven't needed to yet because clang/AST normally has what we need.
> Can this be a method on NamedDecl or a function alongside?
> But since (later comment) the logic seems wrong, I'm not sure it makes sense to move it at the moment. Maybe create a separate header?
Created a separate file. 

Comment at: clangd/SourceCode.h:39
+///  * use spelling location, otherwise.
+SourceLocation getDeclSourceLoc(const clang::Decl* D);
sammccall wrote:
> sammccall wrote:
> > This name seems opaque to me.
> > 
> > findName?
> would it be more useful to return the range, and just pick out the start if you're not interested in both?
Since this function only returns name location, a source location is enough IMO.

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list