kadircet marked 14 inline comments as done. kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/SourceCode.cpp:601 + llvm::inconvertibleErrorCode(), + "File contents differ on disk for %s, please save", FilePath.data()); + } ---------------- sammccall wrote: > you seem to be checking this both here and in clangdlspserver. Why? the contents in here are coming from disk, whereas the one in clangdlspserver is coming from drafts. I suppose it would make more sense to merge the two in clangdlspserver, use draft manager as content source backed by disk in case user hasn't opened that file. ================ Comment at: clang-tools-extra/clangd/SourceCode.h:215 +/// Formats the edits in \p ApplyEdits and generates TextEdits from those. +/// Ensures the files in FS are consistent with the files used while generating ---------------- sammccall wrote: > not sure what "generates TextEdits from those" refers to. > > Could this function be called "reformatEdits" or "formatAroundEdits"? > not sure what "generates TextEdits from those" refers to. Leftover from an old-version. ================ Comment at: clang-tools-extra/clangd/SourceCode.h:220 +llvm::Error formatAndGenerateEdits(llvm::vfs::FileSystem *FS, + llvm::StringMap<Edit> &ApplyEdits, + llvm::StringRef MainFilePath, ---------------- sammccall wrote: > This signature is confusing: we pass code in *three* different ways (FS, > Edits, and MainFileCode). Much of this is because we put the loop (and > therefore all special cases) inside this function. > The logic around the way the VFS is mapped for the main file vs others > doesn't really belong in this layer. Neither does "please save"... > > It seems this wants to be something simpler/smaller like `reformatEdit(const > Edit&, const Style&)` that could be called from ClangdServer. There's > probably another helper like `checkApplies(const Edit&, VFS*)` that would go > in ClangdLSPServer. > as discussed offline for `reformatEdit` to work, made `InitialCode` in `Edit` public. As both formatting and style deduction requires access to file contents. for `checkApplies`, it requires access to both `VFS` and `DraftMgr`and pretty special use-case for clangdlspserver, so don't think the function would carry its weight. Therefore left the check inlined. ================ Comment at: clang-tools-extra/clangd/refactor/Tweak.h:74 + /// A mapping from absolute file path to edits. + llvm::Optional<llvm::StringMap<Edit>> ApplyEdits; ---------------- sammccall wrote: > what's the difference between `None` and an empty map? `None` :P Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66637/new/ https://reviews.llvm.org/D66637 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits