hokein added inline comments.
================ Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:55 + // Include template parameter list. + if (auto *FTD = FD->getDescribedFunctionTemplate()) + return FTD->getBeginLoc(); ---------------- Could you confirm the code handle template specializations as well? I think `getDescribedFunctionTemplate` will return null when FD is a specialization, and we still miss the template list. ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:73 + auto &SM = FD->getASTContext().getSourceManager(); + auto CharRange = toHalfOpenFileRange(SM, FD->getASTContext().getLangOpts(), + FD->getSourceRange()); ---------------- I think we could simplify the code like: ``` const auto* TargetFD = FD->getDescribedFunctionTemplate() ? FD->getDescribedFunctionTemplate(): FD; auto CharRange = toHaleOpenFileRange(.., FD->getSourceRange()); ``` ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:131 + llvm::StringRef FileName = + SM.getFileEntryForID(SM.getMainFileID())->tryGetRealPathName(); + ---------------- should we use `getCanonicalPath` in the `SourceCode.h`? ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:158 + auto InsertionPoint = Region.EligiblePoints.back(); + auto InsertionOffset = positionToOffset(Contents, InsertionPoint); + if (!InsertionOffset) ---------------- nit: maybe put the code for calculating the insertion point to a separate function. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69298/new/ https://reviews.llvm.org/D69298 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
