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

[Nathan James via Phabricator via cfe-commits]

njames93 marked 2 inline comments as done.
njames93 added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:220
+        assert(Attr->getLocation().isValid());
+        if (Attr->getLocation().isMacroID()) {
+          Errors = llvm::joinErrors(
----------------
kadircet wrote:
> 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.
Thanks for explaining that for me, I'll give it a go and see how the test cases are handled


================
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:
> kadircet wrote:
> > again could you please add tests checking this case?
> sorry, i still don't see any cases with multiple virtual keywords.
Ah I thought you meant a test case just containing virtual which there is, I'll get the second one added.
Should I even support multiple virtual decls, https://godbolt.org/z/kGbF22 clang treats this as a warning, but for gcc its an error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75429