sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
LG, thanks! Let me know if/when I should land this for you.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:105
+// argument.
+bool isInMacroNotArg(const SourceManager &SM, const SourceLocation Loc) {
+ return Loc.isMacroID() && !SM.isMacroArgExpansion(Loc);
----------------
This is just SourceManager::isMacroBodyExpansion(), I think.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:163
+ if (isInMacroNotArg(SM, QualifierToRemove.getBeginLoc()) ||
+ isInMacroNotArg(SM, QualifierToRemove.getEndLoc())) {
+ return false;
----------------
I think the endloc check should rather be SM.isWrittenInSameFile - it's no good
if e.g. they're both macro arg expansions, but different ones.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:171
+// The insertion point might be a little awkward if the decl we're anchoring to
+// has a comment in an unfortunate place (e.g. directly above using, or
+// immediately following "namespace {". We should add some helpers for dealing
----------------
nit: you've mentioned the two cases I don't think we should bother fixing, but
not the crucial one: we're anchoring to a regular decl, and we're going to
insert in between the decl and its doc comment.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:245
+ auto &SM = Inputs.AST->getSourceManager();
+ auto TB = Inputs.AST->getTokens();
+ // Determine the length of the qualifier under the cursor, then remove it.
----------------
yikes, this copies the tokenbuffer, I didn't think that was even possible!
auto&
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:44
+ // SourceLocation if the "using" statement already exists.
+ llvm::Expected<InsertionPointData>
+ findInsertionPoint(const Selection &Inputs);
----------------
adamcz wrote:
> sammccall wrote:
> > this doesn't use any state - make it static or a free function?
> It uses Name and NNSL . Would you prefer this as free function, with both
> passed as arguments. My initial thought was that, since prepare() and apply()
> already share these via class members this was fine, but I'm certainly not
> against making this a free function.
Oops, right. I think since this is the complicated part and it has a fairly
simple interface, making its inputs/outputs explicit is nice hygiene.
(prepare and apply do communicate through class state, which feels a bit hacky
to me. We didn't come up with anything nicer but still flexible+simple when
designing this. It is conventional and documented at least)
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:71
+ return true;
+ }
+ if (D->getDeclContext()->Encloses(SelectionDeclContext)) {
----------------
adamcz wrote:
> sammccall wrote:
> > nit: we conventionally leave out the {} on one-line if bodies etc.
> Uhh...is that a hard rule? I personally hate that, it's just asking for Apple
> SSL-style bugs
> https://www.imperialviolet.org/2014/02/22/applebug.html
There's no hard rule, but we do use that style consistently and consistency has
value too.
The clang-tidy check `readability-misleading-indentation` catches that bug. Can
I suggest:
- (for us) adding it to `.clang-tidy` in `llvm-project`? This will affect
clangd and the phabricator linter
- (for the world) we could start a list of on-by-default clang-tidy checks for
clangd users with no `.clang-tidy` config, and include this check
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:89
+
+ // If we're looking at a type or NestedNameSpecifier, walk up the tree until
+ // we find the "main" node we care about, which would be ElaboratedTypeLoc or
----------------
adamcz wrote:
> sammccall wrote:
> > I like the idea here, but I'm not sure it quite works. e.g. any typeloc has
> > a directly enclosing typeloc, the inner one can't be targeted. So what
> > about `x::S*`? (pointertypeloc > elaboratedtypeloc > recordtypeloc)?
> >
> > - looping up from NNS until you get to something else makes sense
> > - unwrapping typeloc -> elaboratedtypeloc makes sense, but I don't think
> > you ever want to unwrap multiple levels?
> > - i don't think these cases overlap at all, you want one or the other
> >
> > Am I missing something?
> If you have a class foo::bar::cc and a struct st inside it, and then do:
> foo::bar::c^c::st *p;
> you end up with:
> PointerTypeLoc -> ElaboratedTypeLoc ->NestedNameSpecifierLoc -> RecordTypeLoc
> in which case you need to go up from type to NNSL and then up again, from
> NNSL to something that's not NNSL.
>
> You have a good point with the PointerTypeLoc, that's a bug. We should stop
> going up the tree as soon as we find ElaboratedTypeLoc. I added a test for
> that.
> foo::bar::c^c::st *p;
But this isn't a case we support, right? We only support extraction when the
NNS consists entirely of namespaces, and such NNSes don't have any children
apart from the qualifier NNS.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:206
+ tooling::Replacements R;
+ if (auto Err = R.add(tooling::Replacement(
+ SM, CharSourceRange::getTokenRange(NNSL.getSourceRange()), "",
----------------
adamcz wrote:
> sammccall wrote:
> > (Sorry if some of this is obvious: TL;DR: we should use TokenBuffer to
> > handle macros in this case)
> >
> > Whenever we try to use SourceLocations to reason about written source code,
> > we have to think about macros. Some libraries try to encapsulate this, but
> > the road to confusion is paved with good intentions, and it's often best to
> > decide on the policy and be explicit about it.
> >
> > For example, when provided a macro location, the tooling::Replacement
> > constructor uses its spelling location.
> > Given:
> > ```
> > // imagine we're trying to abstract away namespaces for C or something
> > #define NS(X) foo::X
> > NS(Bar) boo;
> > ```
> > Running this action on `N` would result in changing the definition of the
> > `NS` macro to delete the "foo::", as that's where the qualifier was spelled!
> >
> > It would be reasonable (but actually not trivial!) to try to detect any
> > macro usage and bail out. The general case of "is there some sequence of
> > source-code tokens that correspond to this range of preprocessed tokens and
> > nothing else" is pretty hard.
> >
> > However TokenBuffer does have APIs for this exact question, as it was
> > designed for writing refactorings that replace nodes. I think you want:
> > - `expandedTokens(NNSL.getSourceRange())`
> > - `spelledForExpanded(ExpandedTokens)`
> > - `Token::range(SM, Spelled.front(), Spelled.back())`
> > - `Replacement("fname", Range.Offset, Range.Length, "")`
> >
> > (ugh, that's really awkward. Maybe we should have a helper in TokenBuffer
> > that combines 1-3)
> >
> You have a good point that this doesn't work well with macros, but I'm not
> entirely sure how you think this should work.
>
> I'd argue that code actions should refer to the thing under the cursor, not
> it's expansion. That would be confusing to the user, as they would not be
> able to understand what the action offered is, nor how it would affect other
> places. So in your example of
> #define NS(X) foo::X
> I'd argue that we should not offer the action.
> We should, however, support something like:
> EXPECT_TRUE(fo^o::bar());
> In this case, the qualifier is spelled under the cursor and the fact that
> EXPECT_TRUE is a macro and not a function should not matter to the user.
>
> I updated the code to support that and added tests.
> We can use isMacroID() and isMacroArgExpansion() to filter out macros, except
> for macro arguments. Note that we can't use spelledForExpanded(), since the
> qualifier might not be the entire expansion of the macro, thus
> spelledForExpanded will return None.
>
> Let me know if any of what I did here now doesn't make sense.
>
> in your example of #define NS(X) foo::X I'd argue that we should not offer
> the action.
Indeed, sorry I didn't spell that out - I was highlighting it because the
original code silently did something (bad) in this case.
> can't use spelledForExpanded(), since the qualifier might not be the entire
> expansion of the macro
Ah, D75446 hasn't landed yet. @kadircet, want to land this soon? :-)
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:226
+
+ if (auto Err = R.add(tooling::Replacement(SM, InsertionPoint->Loc, 0,
+ UsingTextStream.str()))) {
----------------
adamcz wrote:
> sammccall wrote:
> > Again macros.
> > This seems a bit more pressing, there's a nontrivial chance that you're
> > inserting inside a namespace which is introduced by invoking a
> > `FOO_NS_BEGIN` macro.
> >
> > Unfortunately I don't see a really simple way to insert after this macro -
> > using the expansion location fails if macro doesn't end after the `}`.
> > (e.g. `ANON_NAMESPACE(f::X y;)`)
> > (Also you'd need to take care of inserting a space after the macro
> > invocation to avoid `FOO_NS_BEGINusing`.)
> >
> > I'd suggest simply skipping over any candidate locations (namespaces or
> > using declarations) that aren't in the main file, using the expansion
> > location of the first-top-level-decl fallback, and asserting here that the
> > location is in the main file. (We use raw `assert()` in LLVM)
> We already skip over "using" not in main file. I added check for namespace
> and fixed the above-first-top-level-decl to be above getExpandedLoc() of
> that, which works for something like a macro declaring a namespace. It's not
> ideal, but I think it should be good enough, at least for now. Also added a
> test.
>
> Not sure what the point of assert() here would be? Wouldn't it be far better
> to return an error, since it's trivial at this point and it won't crash the
> whole server, including the background index and such? I get that assert()
> can be useful in places where returning error is difficult and for things
> that should never, ever happen, but here seems like an overkill.
>
> Anyway, editMainFile() already returns an error when the edit is not in the
> main file. IMHO that's good enough, what do you think?
Behaviour looks good.
> Not sure what the point of assert() here would be?
This is a consistency check - we're relying on an invariant we think we've
established elsewhere. If this goes wrong, it's a programmer error, and for
better or worse, LLVM defines those as asserts.
Practical benefits:
- developers are more likely to notice when these are violated (assuming an
-UNDEBUG build)
- we get a stacktrace, which is often useful (not really here). Returning an
error doesn't give us that.
- we don't pay for the check in release builds
- it documents the distinction between invariant-breaking bugs and
dynamically-possible error conditions
- consistency (in principle we could change all our err-handling, but we can't
change this in clang)
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:2436
+#include "test.hpp"
+namespace {using one::two::ff;
+
----------------
adamcz wrote:
> sammccall wrote:
> > alternately: `using ::one::two::ff`.
> > Correct more often, uglier/longer, probably a style preference but not
> > worth having an option for. Pick one :-)
> The current code keeps the :: if it was there, but won't add it, so basically
> we respect whatever the user typed. Seems OK to me, do you agree?
>
> I changed one test to include this aspect.
Actually yeah, that's nice and neatly avoids having to make and defend a policy
:-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76432/new/
https://reviews.llvm.org/D76432
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
