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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/SourceCode.cpp:55
+  const auto& SM = D->getASTContext().getSourceManager();
+  SourceLocation SpellingLoc = SM.getSpellingLoc(D->getLocation());
+  if (D->getLocation().isMacroID()) {
----------------
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


================
Comment at: clangd/SourceCode.h:34
 
+/// Get the source location of the given D.
+///
----------------
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?


================
Comment at: clangd/SourceCode.h:36
+///
+/// For symbols defined inside macros:
+///  * use expansion location, if the symbol is formed via macro concatenation.
----------------
Not sure it's neccesary to spell out the details of what's happening inside macros, but it would be useful to describe the normal case :-)
e.g. This is usually the spelling location, where the name of the decl occurs in source code.


================
Comment at: clangd/SourceCode.h:39
+///  * use spelling location, otherwise.
+SourceLocation getDeclSourceLoc(const clang::Decl* D);
+
----------------
This name seems opaque to me.

findName?


================
Comment at: clangd/SourceCode.h:39
+///  * use spelling location, otherwise.
+SourceLocation getDeclSourceLoc(const clang::Decl* D);
+
----------------
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?


================
Comment at: clangd/XRefs.cpp:136
 llvm::Optional<Location>
 getDeclarationLocation(ParsedAST &AST, const SourceRange &ValSourceRange) {
   const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
----------------
The name of this function is *really* confusing, so it took me a long time to understand the callsite.
Consider rename to makeLocaction


================
Comment at: clangd/XRefs.cpp:194
+    auto Loc = getDeclSourceLoc(D);
+    // We use the identifier range as the definition range which matches the
+    // index.
----------------
I think this comment relates to the previous line, not the next one.
But consider just removing it - if getDeclSourceLoc had a clearer name, it'd be self-documenting.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44247