sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/Selection.cpp:621-623
+ if (auto AT = TL->getAs<AttributedTypeLoc>())
+ S = AT.getModifiedLoc()
+ .getSourceRange(); // should we just return false?
----------------
dgoldman wrote:
> Let me know if you think it would be better to return false here
Yes, it's better to return false, which is more conservative. As written, this
code will prevent targeting things within the attribute. (We can't currently
target the attribute itself due to DynTypedNode limitations, but I'd like to
fix that)
================
Comment at: clang-tools-extra/clangd/Selection.cpp:607
SourceRange S = N.getSourceRange();
if (auto *TL = N.get<TypeLoc>()) {
// DeclTypeTypeLoc::getSourceRange() is incomplete, which would lead to
----------------
================
Comment at: clang-tools-extra/clangd/Selection.cpp:619
S.setEnd(DT.getUnderlyingExpr()->getEndLoc());
+ // AttributeTypeLoc points to the attribute's range, NOT the modified
+ // type's range.
----------------
AttributeTypeLoc -> AttributedTypeLoc
This isn't true in general actually - attributes can be prefix or postfix (or
other things...) and the heuristics in TypeLoc::get{Begin,End}Loc basically
assume postfix for `Attributed`, which is right sometimes and wrong sometimes.
(And getSourceRange() doesn't have enough information to get this right - it'd
need the SourceManager to compare locations).
TypeLoc ranges in general seem pretty error prone. I've suggested a comment -
we should consider just bailing out entirely here in future.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2008
+ }},
+ // Should work even with nullability attribute.
+ {
----------------
this is the same as the above case (apart from the selection changes, which are
tested in SelectionTests)
same for the rest of the MYObject tests.
The ones after that are good as they're testing hovering on a different type of
entity.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2052
+ {
+ R"cpp(
+ @class ForwardDeclared;
----------------
these tests seem good (though unrelated to the rest of the patch, you might
want to land them separately).
The Fooey test relies on selection of a protocol in a protocol-list, you may
want to test this directly in SelectionTests.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89579/new/
https://reviews.llvm.org/D89579
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
