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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
