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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D66647#1684012 <https://reviews.llvm.org/D66647#1684012>, @ilya-biryukov wrote:

> It's ok to leave this out of the initial change, but could we describe our strategy to tackle this somewhere in the comments - how we want to fix this and when.


I actually have a fixme saying:

  // FIXME: Instead of fully qualifying we should try deducing visible scopes at
  // target location and generate minimal edits.

Elaborating on it by saying "we can start by using the namespaces in targetcontext".



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:170
+
+    // All Targets should be in the same nested name scope, so we can safely
+    // chose any one of them.
----------------
ilya-biryukov wrote:
> I don't think it's true in presence of using declarations:
> ```
> namespace ns {
>   void foo(int*);
> }
> namespace other_ns {
>   void foo(char*);
> }
> 
> using ns::foo;
> using other_ns::foo;
> 
> template <class T>
> void usages() {
>   foo(T()); // <-- would we add `ns::foo` or `other_ns::foo`?
> }
> ```
> 
> Not qualifying in this case is probably ok, but adding any of the qualifiers is probably wrong.
> Could you check what we do in this case and either add to tests or put into a list of known issues (e.g. file a bug or at least add a FIXME?)
as discussed offline;
action should not be available in that case and we need underlying 


================
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);
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647