[PATCH] D89579: [clangd][ObjC] Support nullability annotations

[Sam McCall via Phabricator via cfe-commits]

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