[PATCH] D75429: [clangd] DefineOutline won't copy virtual specifiers on methods

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D75429#1900709 <https://reviews.llvm.org/D75429#1900709>, @njames93 wrote:

> I'm not entirely sure how to get the spelledTokens working in a good macro safe way?


I don't really follow this comment, could you elaborate? `TB.expandedTokens` always refer to a subset of the pre-processed token stream, so they can contain non-spelled tokens therefore clangd should never try to operate on those tokens. For example:

  #define FOO(X) int X;
  FOO(y);

pre-processed token stream will only contain `int y` which doesn't exist in the source code at all. `TB.spelledForExpanded` tries to map those expanded range back to spelling (to `FOO(y)` for example if you've got the full range `int y`), which is safe to operate on.
if there's no direct mapping between selected range and source code then it returns None, so you should bail out in such cases.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:220
+        assert(Attr->getLocation().isValid());
+        if (Attr->getLocation().isMacroID()) {
+          Errors = llvm::joinErrors(
----------------
njames93 wrote:
> kadircet wrote:
> > can you add some test cases for this branch ? In theory it should be ok to drop this if full expansion is just the attr, i.e.
> > 
> > ```
> > #define FNL final
> > struct A { virtual void foo() FNL {} };
> > ```
> > 
> > but should possibly fail in :
> > ```
> > #define MACRO foo() final
> > struct A { virtual void MACRO {} };
> > ```
> it's not a great idea refactoring functions with MACRO attributes, we can never know if there are side effects based on different definitions due to things like build configs.
well that's definitely one way to go, but while designing this code action we decided user should know better and clangd currently provides this code action in cases involving macros, see `DefineOutlineTest.HandleMacros` and it would be inconsistent with rest of the behavior if it bailed out in half of the cases and kept working for the rest.

Also this case is even safer than the rest, since it is only trying to drop those qualifiers and not duplicate them in the definition side.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:244
+    bool Any = false;
+    // Clang allows duplicating virtual specifiers so check for multiple
+    // occurances.
----------------
kadircet wrote:
> again could you please add tests checking this case?
sorry, i still don't see any cases with multiple virtual keywords.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75429/new/

https://reviews.llvm.org/D75429