[PATCH] D76432: [clangd] Add a tweak for adding "using" statement.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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