[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:77
   //  - if apply() returns an error, returns "fail: <message>"
-  std::string apply(llvm::StringRef MarkedCode) const;
+  std::string apply(llvm::StringRef MarkedCode);
 
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > It seems weird that we have this inconsistency between the contents for current file (passed through the return value) and contents for other files (pass through the fields).
> > 
> > A few better alternatives that come to mind:
> > 1. add an out parameter, could be optional in case when all changes are in the main file.
> > 2. this function will fail when there are changes outside main file, but we can add a new function to return changes in all modified files, e.g. as `StringMap<std::string>` or `vector<pair<Path, string/*Content*/>>` (the latter allows to use standard matchers)
> It is left-out this way since most of the checks only care about a single file, and there are lots of them so changing would definitely require some plumbing.
> 
> I am not sure having the inconsistency between main file and others matters so much though; we have similar discrimination towards non-main files in a bunch of other places in testing infrastructure e.g TestTU.
> 
> As for the suggestions, adding another apply function for multifile case doesn't seem so nice, so I would rather move forward with first one, if you believe this is a strong concern.
> It is left-out this way since most of the checks only care about a single file, and there are lots of them so changing would definitely require some plumbing.
Agree that no matter what we do, we should let the current usages stay the same.

> I am not sure having the inconsistency between main file and others matters so much though; we have similar discrimination towards non-main files in a bunch of other places in testing infrastructure e.g TestTU.

What kind of inconsistency are you referring to? Both main file and extra files are fields inside `TestTU`? It outputs `ParsedAST` and it's a return value of the `build()` function.
There are no multiple ways to return results.

> As for the suggestions, adding another apply function for multifile case doesn't seem so nice, so I would rather move forward with first one, if you believe this is a strong concern.

Yeah, totally ok with this. I'm not a big fan of out parameters myself and prefer putting stuff into the return value instead.
But they're still widely used, so I view this as my preference rather than a common convention (sigh...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647