Nebiroth marked 21 inline comments as done.
Nebiroth added inline comments.
================
Comment at: clangd/ClangdServer.h:306
+ /// Run formatting for \p Rng inside \p File.
+ std::vector<tooling::Replacement> formatRange(PathRef File, Range Rng);
+ /// Run formatting for the whole \p File.
----------------
ilya-biryukov wrote:
> These functions are duplicates of existing ones, they should not be here.
> Bad merge?
Yes, this is a bad merge.
================
Comment at: clangd/ClangdUnit.cpp:438
+ // trimming the extra "::"
+ std::string Res;
+ PrintingPolicy WithScopePP(LangOpts);
----------------
ilya-biryukov wrote:
> We should avoid doing the string-matching when we have the AST.
> Use something like this instead (haven't tested, but you should get the idea):
>
> ```
> /// Returns a NamedDecl that could be used to print the qualifier.
> Decl* getScopeOfDecl(Decl* D) {
> // Code from NamedDecl::printQualifiedName, we should probably move it into
> const DeclContext *Ctx = D->getDeclContext();
>
> // For ObjC methods, look through categories and use the interface as
> context.
> if (auto *MD = dyn_cast<ObjCMethodDecl>(D))
> if (auto *ID = MD->getClassInterface())
> Ctx = ID;
>
> if (Ctx->isFunctionOrMethod()) {
> /// This is a function-local entity.
> return nullptr;
> }
>
> return Ctx;
> }
>
>
> std::string printScopeOfDecl(Decl* Scope) {
> if (isa<TranslationUnitDecl>(Scope))
> return "global namespace";
>
> if (isa<NamedDecl>(Scope))
> return cast<NamedDecl>(Scope)->printQualifiedName();
>
> return "";
> }
>
>
> // Later use like this:
> auto ScopeText = printScopeOfDecl(getScopeOfDecl(D));
> ```
>
I'm not sure I understand. The intention was to have a separate MarkedString
display the scope for whatever we are hovering on. The way I understand the
above code, this would display a scope that isn't as precise as I would like
which defeats the purpose of having this feature in the first place.
================
Comment at: clangd/ClangdUnit.cpp:498
Position Begin;
- Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
- Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
+ Begin.line = SourceMgr.getExpansionLineNumber(LocStart) - 1;
+ Begin.character = SourceMgr.getExpansionColumnNumber(LocStart) - 1;
----------------
ilya-biryukov wrote:
> Why are we changing the semantics here? Why should we use expansion locations
> instead of spelling locations here?
Bad merge.
================
Comment at: clangd/ClangdUnit.cpp:591
+// one line at a time to determine what will be included.
+SourceLocation getFunctionComments(ParsedAST &AST, const Decl *D) {
+
----------------
ilya-biryukov wrote:
> This code seems pretty involved. Don't `RawComment`s handle this case?
Bad merge.
================
Comment at: clangd/ClangdUnit.cpp:968
// createInvocationFromCommandLine sets DisableFree.
+ CI->LangOpts->CommentOpts.ParseAllComments = true;
CI->getFrontendOpts().DisableFree = false;
----------------
ilya-biryukov wrote:
> This should clearly go before the comment!
> Also, we're currently setting this flag when building the preamble, but not
> when parsing the AST. We should definitely do that in both cases.
Do you mean also adding this line somewhere in ASTUnit.cpp?
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
