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

[Haojian Wu via Phabricator via cfe-commits]

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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44247