ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed.
This is probably not a final version, but I feel my comments should be helpful anyway. Also, we're missing testcases now. ================ Comment at: clangd/ClangdLSPServer.cpp:225 + PathRef ref = PathRef((Params.uri.uri).c_str()); + Path result = LangServer.Server.switchSourceHeader(ref); + ---------------- We use `Params.uri.file`, not `Params.uri.uri` when passing paths to clangd. Also no need for extra local var: `LangServer.Server.switchSourceHeader(Params.uri.file);` should work. ================ Comment at: clangd/ClangdLSPServer.cpp:225 + PathRef ref = PathRef((Params.uri.uri).c_str()); + Path result = LangServer.Server.switchSourceHeader(ref); + ---------------- ilya-biryukov wrote: > We use `Params.uri.file`, not `Params.uri.uri` when passing paths to clangd. > Also no need for extra local var: > `LangServer.Server.switchSourceHeader(Params.uri.file);` should work. Naming: local vars should start with capital letter. ================ Comment at: clangd/ClangdServer.cpp:295 + std::string DEFAULT_SOURCE_EXTENSIONS[] = { ".cpp", ".c", ".cc", ".cxx", + ".c++", ".C", ".m", ".mm" }; + std::string DEFAULT_HEADER_EXTENSIONS[] = { ".h", ".hh", ".hpp", ".hxx", ---------------- We should check all extensions in both upper-case and lower-case, not just `.c` and `.C` (ideally, we could use case-insensitive comparisons). ================ Comment at: clangd/ClangdServer.cpp:302 + + std::string pathDataRef = std::string(path.data()); + bool isSourceFile = false, foundExtension = false; ---------------- `path` is already a `StringRef`, no need to convert it. `std::string` is also implicitly convertible to `StringRef`, you could pass `std::string` to every function that accepts a `StringRef` ================ Comment at: clangd/ClangdServer.cpp:307 + + while (!foundExtension && i < sourceSize) + { ---------------- Maybe replace with: ``` if (std::find(DEFAULT_SOURCE_EXTESIONS, /*end iterator*/, llvm::sys::path::extensions(path)) { isSourceFile = true; foundExtension = true; } ``` ================ Comment at: clangd/ClangdServer.cpp:323 + { + while (!foundExtension && i < headerSize) + { ---------------- Maybe use `std::find` here too? ================ Comment at: clangd/ClangdServer.cpp:339 + SmallString<128> CurrentPath; + CurrentPath = temp; + bool done = false; ---------------- Don't need `temp` var here, there's `StringRef::toVector` method to convert `StringRef` to `SmallString`. ================ Comment at: clangd/ClangdServer.cpp:341 + bool done = false; + std::vector<std::string> EXTENSIONS_ARRAY; + ---------------- Naming: EXTENSIONS_ARRAY is a local var, use `UpperCamelCase`. Maybe use `ArrayRef<std::string>` for `ExtensionsArray` instead? ================ Comment at: clangd/ClangdServer.cpp:360 + FILE * file; + file = fopen(temp.c_str(), "r"); + if (file) ---------------- Please use `vfs::FileSystem` instance, provided by `FSProvider.getTaggedFileSystem()` here for file accesses. ================ Comment at: clangd/ClangdServer.cpp:367 + std::string returnValue = std::string(NewPath.str()); + return Path("\"" + returnValue + "\""); + } ---------------- Don't add quotes here, `Uri` constructor and `Uri::unparse` is responsible for doing that. ================ Comment at: clangd/ClangdServer.h:185 + std::string switchSourceHeader(std::string path); + ---------------- ilya-biryukov wrote: > ilya-biryukov wrote: > > Please use descriptive typedefs for paths: `Path` (equivalent to > > `std::string`) for return type and `PathRef` (equivalent to `StringRef`) > > for parameter type. > Maybe add a small comment for this function to indicate it's a fairly trivial > helper? Maybe also change wording to indicate that this function does not mutate any state and is indeed trivial, i.e.: `Helper function that returns a path to the corresponding source file when given a header file and vice versa.` Code style: please end all comments with full stops. ================ Comment at: clangd/ClangdServer.h:186 + // Helper function to change from one header file to it's corresponding source file and vice versa + Path switchSourceHeader(PathRef path); + ---------------- Naming: parameter names must be capitalized ================ Comment at: clangd/ProtocolHandlers.cpp:260 + Dispatcher.registerHandler( + "textDocument/switchSourceHeader", + llvm::make_unique<SwitchSourceHeaderHandler>(Out, Callbacks)); ---------------- ilya-biryukov wrote: > I don't see this method in the [[ > https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md > | LSP spec ]]. > Is this some kind of extension? > > Could you add some comments on that with pointers to proposal/explanation of > where this extension is used? This comment was not addressed, probably marked as 'Done' by accident. https://reviews.llvm.org/D36150 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits