[PATCH] D36150: [clangd] LSP extension to switch between source/header file

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

The function in `ClangdServer` seems to be getting more complicated and more complicated, mostly because of mixing up `std::string`, `StringRef`, `SmallString`.
Here's a slightly revamped implementation of your function, hope you'll find it (or parts of it) useful to simplify your implementation and address remaining comments.

  // Note that the returned value is now llvm::Optional to clearly indicate no matching file was found.
  llvm::Optional<Path> ClangdServer::switchSourceHeader(PathRef Path) {
    const StringRef SourceExtensions[] = {/*...*/; // only lower-case extensions
    const StringRef HeaderExtensions[] = {/*...*/}; // ...
  
    StringRef PathExt = llvm::sys::path::extension(Path);
    
    // Lookup in a list of known extensions.
    auto SourceIter = std::find(SourceExtensions, /*end iterator*/, PathExt, [](StringRef LHS, StringRef RHS) { return LHS.equals_lower(RHS); });
    bool IsSource = SourceIter != /*end iterator*/;
    
    // ...
    bool IsHeader = ...;
  
    // We can only switch between extensions known extensions.
    if (!IsSource && !IsHeader)
      return llvm::None;
  
    // Array to lookup extensions for the switch. An opposite of where original extension was found.
    ArrayRef<StringRef> NewExts = IsSource ? HeaderExtensions : SourceExtensions;
  
    // Storage for the new path.
    SmallString<128> NewPath;
    Path.toVector(NewPath);
  
    // Instance of vfs::FileSystem, used for file existence checks.
    auto FS = FSProvider.getTaggedFileSystem(Path).Value;
  
    // Loop through switched extension candidates.
    for (StringRef NewExt : NewExts) { 
      llvm::sys::path::replace_extension(NewPath, NewExt)
      if (FS->exists(NewPath))
        return NewPath.str().str(); // First str() to convert from SmallString to StringRef, second to convert from StringRef to std::string
  
      // Also check NewExt in upper-case, just in case.
      llvm::sys::path::replace_extension(NewPath, NewExt.upper())
      if (FS->exists(NewPath))
        return NewPath.str().str();
    }
  
    return llvm::None;
  }



================
Comment at: clangd/ClangdServer.cpp:404
+        std::string returnValue = "file://" + std::string(test.str());
+        returnValue = URI::unparse(URI::fromUri(returnValue));
+        return Path(returnValue);
----------------
No need to create a `URI` here, this should be handled outside `ClangdServer`, just return a path with replaced extension.


https://reviews.llvm.org/D36150