Page MenuHomePhabricator

Adding inline comments to code view type records
AcceptedPublic

Authored by nilanjana_basu on Jul 10 2019, 3:09 PM.

Details

Reviewers
rnk
Summary

Code view debug info type records are emitted as a set of directives. Comments describing these records were previously emitted all together for each record at the end of the record. With this change, every directive representing a sub-field of a type record is augmented with a comment describing the sub field.

Diff Detail

Repository
rL LLVM

Event Timeline

nilanjana_basu created this revision.Jul 10 2019, 3:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2019, 3:09 PM
rnk added inline comments.Jul 10 2019, 5:42 PM
llvm/include/llvm/DebugInfo/CodeView/CodeViewRecordIO.h
63

Is it possible to change these all to accept const Twine &Comment = ""? I think string literals are meant to be implicitly convertible to Twine.

Generally in LLVM StringRef is preferred over const char *. We have some documentation for this here:
https://llvm.org/docs/ProgrammersManual.html#passing-strings-the-stringref-and-twine-classes

I think for this API where the string length is unlikely to be used at all, Twine makes sense, but across LLVM C strings are generally avoided if possible.

String parameter passing is done using Twine instead of using C strings, as suggested in the review

Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2019, 11:38 AM
nilanjana_basu marked 2 inline comments as done.Jul 11 2019, 11:39 AM
nilanjana_basu added inline comments.
llvm/include/llvm/DebugInfo/CodeView/CodeViewRecordIO.h
63

Got it. Have made the changes & updated the patch.

nilanjana_basu marked an inline comment as done.Jul 16 2019, 9:44 AM

Corrected comments for codeview type record from "NumAttributes" to "AccessSpecifier"

Added record kind name in the comments for better readability

rnk accepted this revision.Jul 17 2019, 10:18 AM

lgtm, sorry this fell out of my inbox.

This revision is now accepted and ready to land.Jul 17 2019, 10:18 AM