This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Added highlightings for namespace specifiers.
ClosedPublic

Authored by jvikstrom on Jul 10 2019, 7:09 AM.

Event Timeline

jvikstrom created this revision.Jul 10 2019, 7:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2019, 7:09 AM
hokein added inline comments.Jul 10 2019, 8:02 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
41

nit: const auto *

42

nit: you miss a return

73

this comment is stale with this patch, now we are highlighting namespace, it seems more natural to do it here, we can get a NestedNameSpecifierLoc from an ElaboratedTypeLoc (so that we don't need the TraverseNestedNameSpecifierLoc).

247

I think here should be entity.name.namespace.cpp, vscode uses this TX scope for namespace.

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
114

could you add a testcase for anonymous namespace?

namespace {}

119

nit: the static is not needed for a type.

jvikstrom updated this revision to Diff 208998.Jul 10 2019, 9:59 AM
jvikstrom marked 6 inline comments as done.

Addressed comments.

jvikstrom added inline comments.Jul 10 2019, 10:01 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
73

Doing it in VisitTypeLoc means that we fail on this testcase:

namespace $Namespace[[aa]] {
  namespace $Namespace[[bb]] {
    struct $Class[[A]] {};
  }
}
$Namespace[[aa]]::$Namespace[[bb]]::$Class[[A]] $Variable[[a]];

It can't detect the bb namespace qualifier in aa::bb::A a;.

Maybe there is some way of solving this without TraverseNestedNameSpecifierLoc that I am not aware of?
Also don't know how I'd get a NestedNameSpecifierLoc from an ElaboratedTypeLoc.

247

Really? Because I see entity.name.type.namespace.cpp when I inspect the TM scopes for namespaces.

sammccall added inline comments.Jul 10 2019, 11:12 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
73

TraverseNestedNameSpecifierLoc is the right way to deal with namespaces-as-prefixes, I think it's best to follow that consistently.

(For namespaces not-as-prefixes, you've got namespacealiasdecl, namespacedecl, using-directives... I think that's everything)

85

if you're just doing something and then calling base, can you make this Visit instead of Traverse?

129

this doesn't fit with the contract of this addToken() function, which should only highlight the provided token.

Instead, can you call this from VisitNamespaceAliasDecl?

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
86

can you add a case where we spell with leading colons e.g. ::abc::A

jvikstrom marked 4 inline comments as done.

Moved alias target namespace add token to another function and added testcase for global namespace specifier.

jvikstrom added inline comments.Jul 11 2019, 1:03 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
85

NestedNameSpecifierLoc does not have a visit method for some reason. It only has a traverse method and this seems to be the way people get nested namespaces from what I can tell. (Maybe a Visit method is something that should be added to the RecursiveASTVisitor, but I wouldn't do that in this patch).

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
86

Added it in the case that focuses on namespaces further down.

hokein accepted this revision.Jul 11 2019, 1:39 AM

looks good from my side, just a comment for the TM scope name.

clang-tools-extra/clangd/SemanticHighlighting.cpp
73

thanks for the explanation, fair enough.

247

that's weird, TM inspector told me it was entity.name.namespace.cpp, I can also find it in https://github.com/microsoft/vscode/blob/master/extensions/cpp/syntaxes/cpp.tmLanguage.json#L1709

This revision is now accepted and ready to land.Jul 11 2019, 1:39 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2019, 2:30 AM