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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Currently we add too many qualifiers in some cases, e.g. here's a common pattern in clangd:

  // foo.h
  #include "Decl.h"
  namespace clang::clangd { std::string printName(Decl*D); }
  
  // foo.cpp
  #include "foo.h"
  namespace clang::clangd { 
    std::string printName(Decl *D) {
      NamedDecl* ND = ...;
    } 
  ...
  }

this will result in:

  // foo.h
  namespace clang::clangd {
  std::string printName(Decl*D) {
    clang::NamedDecl *ND = ...; // <-- (!) we did not even had any 'using' declarations or 'using namespace's 
  }
  }

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.



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647