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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
