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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
