Page MenuHomePhabricator

[clangd] Print template arguments helper
ClosedPublic

Authored by kadircet on Mar 21 2019, 6:41 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

kadircet created this revision.Mar 21 2019, 6:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2019, 6:41 AM
Eugene.Zelenko added inline comments.
clang-tools-extra/clangd/AST.cpp
26 ↗(On Diff #191677)

Functions should be static, not in anonymous namespace. See LLVM Coding Guidelines.

28 ↗(On Diff #191677)

Return type is not obvious, so auto should not be used.

131 ↗(On Diff #191677)

Return type is not obvious, so auto should not be used.

clang/lib/AST/TypePrinter.cpp
1642 ↗(On Diff #191677)

Return type is not obvious, so auto should not be used.

ioeric added inline comments.Mar 21 2019, 10:35 AM
clang-tools-extra/clangd/AST.cpp
88 ↗(On Diff #191677)

can we unify this with getTemplateSpecializationArgLocs somehow?

it seems that args as written would also be favorable here (see FIXME in line 112)

131 ↗(On Diff #191677)

nit: I'd suggest keeping {} for symmetry.

133 ↗(On Diff #191677)

why isn't this handled in getTemplateSpecializationArgLocs? Add a comment?

134 ↗(On Diff #191677)

could you add comment/example explaining what's special about this case?

clang-tools-extra/clangd/AST.h
54 ↗(On Diff #191677)

Maybe printTemplateSpecializationArgs since we are only handling template specialization?

I think we could drop AsWritten from the name. It should be clear to just explain in the comment.

clang-tools-extra/unittests/clangd/ASTUtilsTests.cpp
16 ↗(On Diff #191677)

I think this kind of tests would be more readable using TEST_P [1] with code and expected arguments as parameters.

Up to you though.

[1] https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#specifying-names-for-value-parameterized-test-parameters

clang/lib/AST/TypePrinter.cpp
1635 ↗(On Diff #191677)

could you add clang tests for these changes?

1643 ↗(On Diff #191677)

why?

/// Represents an empty template argument, e.g., one that has not
/// been deduced.

It seems legitimate.

Since this hot code path, we would want to avoid hard assertion if possible.

ioeric added inline comments.Mar 21 2019, 10:46 AM
clang/lib/AST/TypePrinter.cpp
1640 ↗(On Diff #191677)

It's unclear to me what the new behavior is with changes in this file. Could you add comment?

It might make sense to split the clang change into a separate patch and get folks who are more familiar to take a look.

ilya-biryukov added inline comments.Mar 22 2019, 2:50 AM
clang-tools-extra/unittests/clangd/ASTUtilsTests.cpp
1 ↗(On Diff #191677)

NIT: add a licence header

clang-tools-extra/unittests/clangd/CMakeLists.txt
13 ↗(On Diff #191677)

NIT: maybe call it PrintASTTests?
To avoid creating a dump of test for all the stuff we are too lazy to classify properly

ilya-biryukov added inline comments.Mar 22 2019, 3:02 AM
clang-tools-extra/clangd/AST.cpp
26 ↗(On Diff #191677)

We tend to put internal functions to anonymous namespace quite a lot in clangd.
While technically a small violation of the LLVM style guide, this aligns with the rest of the code and we don't consider that to be a problem.

kadircet marked 15 inline comments as done.Mar 22 2019, 4:30 AM
kadircet added inline comments.
clang-tools-extra/clangd/AST.cpp
88 ↗(On Diff #191677)

See D59641

133 ↗(On Diff #191677)

it is mentioned in getTemplateSpecializationArgLocs, would you rather move the comment here?

clang/lib/AST/TypePrinter.cpp
1635 ↗(On Diff #191677)

We've also discussed this in the original comment and decided infrastructure necessary was too overwhelming. First we need to invoke compiler and get the AST, then write a Visitor to fetch relevant decls and only after that call printTemplateArguments ...

1640 ↗(On Diff #191677)

it just prevents erasure from TypeLoc to Type when printing the argument, so that we can do a fine-grained printing if Location is already there.

1643 ↗(On Diff #191677)

Sure, letting TemplateArgument::print handle that

kadircet updated this revision to Diff 191857.Mar 22 2019, 4:30 AM
kadircet marked an inline comment as done.
  • Address comments
Eugene.Zelenko added inline comments.Mar 22 2019, 6:47 AM
clang-tools-extra/clangd/AST.cpp
26 ↗(On Diff #191677)

I don't think it's reasonable to have one style per project. Even if clangd has such historical code, it'll be good idea to change it to confirm to common guidelines. Evolution of LLDB formatting style is good example.

ioeric added inline comments.Mar 22 2019, 8:32 AM
clang-tools-extra/clangd/AST.cpp
149 ↗(On Diff #191857)

I'm not quite sure if I understand this FIXME. In this else branch, we are handling this case. Do you mean this is not a proper fix and future work is needed here? Could you elaborate?

One thing that's worth commenting is why we could use Cls->getTemplateArgs().asArray() when Cls->getTypeAsWritten is null. It's not trivial from the code.

88 ↗(On Diff #191677)

thought?

133 ↗(On Diff #191677)

I think it would be clearer if we do something like:

if (auto *Cls = llvm::dyn_cast<ClassTemplateSpecializationDecl>(&ND)) {
  // handle this specially because ...
} else {
  // use getTemplateSpecializationArgLocs to handle rest of cases.
}
clang/lib/AST/TypePrinter.cpp
1635 ↗(On Diff #191677)

No test at all is concerning... I think there are lit tests for AST printing in test/AST/. Is it possible to have some of those?

1640 ↗(On Diff #191677)

could you add this into comment?

kadircet updated this revision to Diff 194657.Apr 11 2019, 3:35 AM
kadircet marked 7 inline comments as done.
  • Address comments
clang-tools-extra/clangd/AST.cpp
149 ↗(On Diff #191857)

Yes, this should rather be fixed in AST itself. Updating comment to explain the fallback strategy.

133 ↗(On Diff #191677)

I am not performing this in the order you mentioned since ClassTemplatePartialSpecializationDecl is also a ClassTemplateSpecializationDecl, but if you insist I can change the ordering by adding another condition(which just looks ugly).

kadircet updated this revision to Diff 194658.Apr 11 2019, 3:40 AM
  • Update file comment for PrintASTTests.cpp
ioeric accepted this revision.Apr 11 2019, 4:48 AM

lgtm

clang-tools-extra/clangd/AST.cpp
139 ↗(On Diff #194658)

Could you also add a test case for this with the current behavior and a FIXME?

clang-tools-extra/unittests/clangd/PrintASTTests.cpp
47 ↗(On Diff #194658)

maybe s/TemplateArgs/TemplateArgsAtPoints/ for clarity

This revision is now accepted and ready to land.Apr 11 2019, 4:48 AM
kadircet updated this revision to Diff 194686.Apr 11 2019, 7:52 AM
kadircet marked 6 inline comments as done.
  • Address comments
kadircet added inline comments.Apr 11 2019, 7:52 AM
clang-tools-extra/clangd/AST.cpp
139 ↗(On Diff #194658)

Adding the test case, but the problem is there wouldn't be a change in behavior(at least not in the cases that I can think of) since a full specialization doesn't have any dependent types.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2019, 3:10 AM
thakis added inline comments.
clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
16

Keep alphabetized?