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

Mon Mar 2 05:52:10 PST 2020

kadircet added inline comments.

================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:244
+    bool Any = false;
+    // Clang allows duplicating virtual specifiers so check for multiple
+    // occurances.
----------------
njames93 wrote:
> 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.
If compiler has an error parsing those, we'll be on the safe side since AST will be broken for that node and we'll most likely bail out long before we reach here.

If compiler is happy with those, and produces just warnings, deleting one vs deleting multiple shouldn't make much difference in the code path(just a matter of breaking or not), but would make action more resilient so I would go for deleting multiple, since it won't have any side effects the user will still see the compiler warning in the header(declaration location).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75429