[PATCH] D66637: [clangd] Support multifile edits as output of Tweaks

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 13 inline comments as done.
kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:414
+      if (llvm::Error Err = reformatEdit(E, Style)) {
+        llvm::handleAllErrors(std::move(Err), [&](llvm::ErrorInfoBase &EIB) {
+          elog("Failed to format {0}: {1}", It.first(), EIB.message());
----------------
sammccall wrote:
> isn't this just
> `elog("Failed to format {0}: {1}", std::move(Err))`
IIUC, just printing the error doesn't actually mark it as checked, therefore causes an assertion failure (see `getPtr` vs `getPayload` in `llvm::Error`). And I couldn't see any special handling in `elog`, but not sure if `formatv object` has this.


================
Comment at: clang-tools-extra/clangd/refactor/Tweak.cpp:90
+  assert(FE && "Generating edits for unknown file!");
+  llvm::StringRef FilePath = FE->getName();
+  Edit Ed(SM.getBufferData(FID), std::move(Replacements));
----------------
sammccall wrote:
> how do you know this is the correct/absolute path for the file? Can we add tests?
changed it to use `FileInfo::getName` rather then `FileEntry::getName`, as the former says:
```
/// Returns the name of the file that was used when the file was loaded from
/// the underlying file system.```

These also get tested through any lit tests we have for tweaks, adding more unittests.


================
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:130
+/// pointed by FID.
+Tweak::Effect fileEdit(const SourceManager &SM, FileID FID,
+                       tooling::Replacements Replacements);
----------------
sammccall wrote:
> sammccall wrote:
> > what will this be used for?
> > 
> > You can use at most one of these Effect factories (unless we're going to do some convoluted merge thing), so this seems useful if you've got exactly one edit to a non-main file.
> > 
> > It seems more useful to return a pair<string, Edit> which could be inserted into a map
> why are these moved out of the Effect class?
> need to provide a fully-qualified name (I don't think the implementations in this patch actually do that)

I've thought you were also requesting these to be helper functions rather than static methods so that callers don't need to qualify the call.
Can move it back into class if I misunderstood your point, but this actually seems a lot cleaner.


================
Comment at: clang-tools-extra/clangd/refactor/Tweak.h:130
+/// pointed by FID.
+Tweak::Effect fileEdit(const SourceManager &SM, FileID FID,
+                       tooling::Replacements Replacements);
----------------
kadircet wrote:
> sammccall wrote:
> > sammccall wrote:
> > > what will this be used for?
> > > 
> > > You can use at most one of these Effect factories (unless we're going to do some convoluted merge thing), so this seems useful if you've got exactly one edit to a non-main file.
> > > 
> > > It seems more useful to return a pair<string, Edit> which could be inserted into a map
> > why are these moved out of the Effect class?
> > need to provide a fully-qualified name (I don't think the implementations in this patch actually do that)
> 
> I've thought you were also requesting these to be helper functions rather than static methods so that callers don't need to qualify the call.
> Can move it back into class if I misunderstood your point, but this actually seems a lot cleaner.
> what will this be used for?

you are right it doesn't really compose well, changed it to return the pair instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66637/new/

https://reviews.llvm.org/D66637