[PATCH] D71596: [clangd] Improve documentation for auto and implicit specs

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Dec 18 02:07:30 PST 2019


ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:188
+// returns D.
+const NamedDecl *getExplicitSpec(const NamedDecl *D) {
+  if (auto *CTSD = llvm::dyn_cast<ClassTemplateSpecializationDecl>(D)) {
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > kadircet wrote:
> > > ilya-biryukov wrote:
> > > > What's the purpose of this function?
> > > > I don't think its description has semantic meaning in C++, "implicit instantiations" do not have an "explicit specialization"...
> > > > 
> > > > It seems to be doing something to change a decl into something that could be used to query the index. If that's the case, we could probably have a name that's closer to the described goal.
> > > yeah naming is hard :/
> > > 
> > > it is not just something that can be used to query index, but also for AST, as the decl of instantiation doesn't contain comments attached to specialization.
> > > this is basically returning the explicit specialization used to instantiate `D`.
> > > 
> > > any suggestions for the name ?
> > I would go with the following (a different comment is welcome, just quickly came up with something):
> > ```
> > /// Returns a decl that should be used to produce hover, i.e. the one
> > /// we should query for documentation comment and use in index queries.
> > Decl *getDeclForHoverInfo(Decl *D) 
> > ```
> > 
> > The name is not smart and one would definitely need to read the comment to understand why we need it in the first place, but at least it would make sure we avoid confusion.
> the problem is, this is not the decl used to produce hover exactly (for example we want to make use of implicit instantiation for printing the name).
> we only want to make use of it for comment retrieval, modifying according to that.
If it's just for the comment, `getDeclForComment` is perfect :-)


================
Comment at: clang-tools-extra/clangd/Hover.cpp:189
+const NamedDecl *getDeclForComment(const NamedDecl *D) {
+  // Return explicit specialization for implicit instantiations, as comments are
+  // attached to that.
----------------
There's no such thing as `explicit specialization for implicit instantiations`.
Maybe remove this comment? It merely duplicates the code and it doesn't really matter what happens there, as long as we get the comment in hover.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71596





More information about the cfe-commits mailing list