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