kadircet added inline comments.
================
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()), "",
----------------
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
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:249
+ const auto *SpelledBeginTok =
+ TB.spelledTokenAt(SM.getSpellingLoc(ExpandedTokens.front().location()));
+ const auto *SpelledEndTok =
----------------
why not use spelledForExpanded in here as well?
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
