This is an archive of the discontinued LLVM Phabricator instance.

[clang-doc] Attaching a name to reference data
ClosedPublic

Authored by juliehockett on Apr 30 2018, 12:09 PM.

Details

Summary

This adds the name of the referenced decl, in addition to its USR, to the saved data, so that the backend can look at an info in isolation and still be able to construct a human-readable name for it.

Diff Detail

Repository
rL LLVM

Event Timeline

juliehockett created this revision.Apr 30 2018, 12:09 PM
lebedev.ri added inline comments.Apr 30 2018, 12:22 PM
clang-doc/BitcodeWriter.cpp
275 ↗(On Diff #144607)
  1. I'm not seeing where this is being changed?
  2. This looks like some abstraction is missing. Maybe at least put it as private member of ClangDocBitcodeWriter?
clang-doc/BitcodeWriter.h
34 ↗(On Diff #144607)

Looking at the changes, may want to bump the version.

64 ↗(On Diff #144607)

General observation: you could keep BI_LAST element as the last one in the enum (just like the rest,
without specifying it's value), and use it just like the end(), i.e. v < BI_LAST instead of v <= BI_LAST.

juliehockett marked 3 inline comments as done.

Addressing comments

Global question: you are using NamedDecl::getNameAsString(), and passing it as StringRef.
You have looked at this with ASAN, and it's ok?

Also, have you considered using the NamedDecl::getName(), which already returns StringRef.?

clang-doc/Serialize.cpp
185 ↗(On Diff #144615)

This change from InfoType::IT_record is intentional?

200–217 ↗(On Diff #144615)

This very closely matches the parseFields(), with a few changes (I.Params vs I.Members,
getUSRForDecl(T) vs getUSRForDecl(N), F->getQualifiedNameAsString() vs P->getQualifiedNameAsString()).
If it makes sense, it might be a good idea to refactor them together?

lebedev.ri added inline comments.May 1 2018, 3:12 AM
clang-doc/BitcodeWriter.cpp
382 ↗(On Diff #144615)

Global question: you are using NamedDecl::getNameAsString(), and passing it as StringRef.
You have looked at this with ASAN, and it's ok?

Also, have you considered using the NamedDecl::getName(), which already returns StringRef.?

Hm, looking at those two functions, not sure NamedDecl::getName() will work here.
Alternatively, have you considered just making this Name field store DeclarationName,
and call getNameAsString() only here?

juliehockett marked 3 inline comments as done.

Addressing a fixme and updating tests

clang-doc/BitcodeWriter.cpp
382 ↗(On Diff #144615)

The field stores it as a string? So the name string is copied into the info data structure itself -- this is so that the backend need have no knowledge of the AST to do its job.

clang-doc/Serialize.cpp
200–217 ↗(On Diff #144615)

So the fixme in parseFields was the main difference -- I fixed it, and they are subtly different (that is, parseFields adds a MemberTypeInfo with an access attached to it, where as parseParameters adds a FieldTypeInfo without the access attached). Also, the way to get the information we want from a ParmVarDecl is slightly different from how you get it from a FieldDecl.

lebedev.ri accepted this revision.May 1 2018, 10:33 AM

I can't really comment on the BC change since i don't think anything
here makes use of that yet, but the code changes look ok to me.

LGTM.

clang-doc/BitcodeWriter.cpp
382 ↗(On Diff #144615)

Oh i see, forgot about that.

clang-doc/BitcodeWriter.h
146 ↗(On Diff #144747)

There is llvm::SmallDenseMap, apparently. Not sure if it helps here.

This revision is now accepted and ready to land.May 1 2018, 10:33 AM
lebedev.ri added inline comments.May 1 2018, 3:34 PM
clang-doc/BitcodeWriter.cpp
382 ↗(On Diff #144615)

One more thing to consider, then: does it always receive std::string?
(from getNameAsString() and others, maybe).

If yes, maybe it would be simpler, more efficient (Since you could std::move() it,
no copying/allocating needed.) to just store the std::string itself.

This revision was automatically updated to reflect the committed changes.
juliehockett marked 2 inline comments as done.