adamcz added inline comments.
================
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
----------------
sammccall wrote:
> 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.
Not right now, but I left a comment that we should improve that. The way it is
now, code is correct, always, and improving it is just a matter or removing the
"no record types" check and replacing with a bit more code. Seems better to me.
================
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()), "",
----------------
kadircet wrote:
> sammccall wrote:
> > 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? :-)
> >
> >
> done, let me know if it doesn't work :D
Works great, thanks!
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:226
+
+ if (auto Err = R.add(tooling::Replacement(SM, InsertionPoint->Loc, 0,
+ UsingTextStream.str()))) {
----------------
sammccall wrote:
> 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)
OK, I added the assert(). I'm not against asserts in general, I just find them
often insufficient and not a replacement for better error handling. In this
case, it certainly won't hurt.
================
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.
----------------
sammccall wrote:
> sammccall wrote:
> > yikes, this copies the tokenbuffer, I didn't think that was even possible!
> > auto&
> Copy constructor deleted in 94938d7d41cd11c4539ff93b801fe53cb4fddba2
Haha, that's a lovely bug. Thanks for fixing the ctor.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:249
+ const auto *SpelledBeginTok =
+ TB.spelledTokenAt(SM.getSpellingLoc(ExpandedTokens.front().location()));
+ const auto *SpelledEndTok =
----------------
kadircet wrote:
> why not use spelledForExpanded in here as well?
It didn't work before. Thanks for improving it!
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
