This is an archive of the discontinued LLVM Phabricator instance.

Add data formatters for go strings and slices.
ClosedPublic

Authored by ribrdb on Oct 19 2015, 3:11 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

ribrdb updated this revision to Diff 37799.Oct 19 2015, 3:11 PM
ribrdb updated this revision to Diff 37800.
ribrdb retitled this revision from to Add data formatters for go strings and slices..
ribrdb updated this object.
ribrdb added a reviewer: clayborg.
ribrdb set the repository for this revision to rL LLVM.
ribrdb added a subscriber: lldb-commits.

Add tests.

clayborg resigned from this revision.Oct 19 2015, 3:35 PM
clayborg edited reviewers, added: granata.enrico; removed: clayborg.

Enrico should take a look and OK this.

granata.enrico added inline comments.Oct 19 2015, 4:40 PM
source/Plugins/Language/Go/GoFormatterFunctions.cpp
104 ↗(On Diff #37800)

This worries me a little bit

Imagine I have one of these slices with a million elements, and the user gets in and the first thing they do is GetChildAtIndex(750000); then GetChildAtIndex(134621); then GetChildAtIndex(999999)

Now I have allocated one million shared pointers, even though only three are in use

How about std::map<size_t, ValueObjectSP>?

145 ↗(On Diff #37800)

I would also do options.SetLanguage(eLanguageTypeGo) here - in case you ever want to do custom escaping, or the StringPrinter gains any further language-specific abilities

source/Plugins/Language/Go/GoLanguage.cpp
79 ↗(On Diff #37800)

Any reason why you need to use hardcoded formatters here?

Do strings and slices not have simple type names to match against in Go?

If at all possible, I would prefer to see you add formatters by name instead of hardcoded matches. Hardcoded matches are really meant for cases where the predicate you're trying to express is something a type name or regular expression on type names can't capture.

Eugene.Zelenko added inline comments.
source/Plugins/Language/Go/GoFormatterFunctions.cpp
10 ↗(On Diff #37800)

Please add include section comments. Same for other sections.

98 ↗(On Diff #37800)

Please place destructor near constructor.

source/Plugins/Language/Go/GoLanguage.cpp
49 ↗(On Diff #37800)

Please use consistent spacing between methods.

source/Plugins/Language/Go/GoLanguage.h
31 ↗(On Diff #37800)

Please use override instead of virtual.

34 ↗(On Diff #37800)

Please place constructor before destructor.

65 ↗(On Diff #37800)

virtual is not needed. Please run clang-tidy modernize-use-override.

Same for GetPluginVersion().

Eugene.Zelenko added inline comments.Oct 19 2015, 6:45 PM
source/Plugins/Language/Go/GoFormatterFunctions.h
1 ↗(On Diff #37800)

Such comments should be 80 characters long.

15 ↗(On Diff #37800)

Please add headers sections comments. Same for other sections. Clang headers should be before LLDB.

source/Plugins/Language/Go/GoLanguage.cpp
11 ↗(On Diff #37800)

Please add headers sections comments. GoLanguage.h should be in LLDB headers section.

I think will be good idea to format code with Clang-format.

See also rL250789 I just committed with similar cleanups for other languages.

Eugene.Zelenko added inline comments.Oct 19 2015, 6:57 PM
source/Plugins/Language/Go/GoFormatterFunctions.cpp
107 ↗(On Diff #37800)

One space should be enough.

ribrdb added inline comments.Oct 20 2015, 10:54 AM
source/Plugins/Language/Go/GoFormatterFunctions.cpp
10 ↗(On Diff #37800)

Which sections? I've seen a couple types of section markers in different files, but it's not consistent. Are there documented recommendations for these?

source/Plugins/Language/Go/GoLanguage.cpp
79 ↗(On Diff #37800)

Normal string and slice types can be recognized by name.
But it is easy to create other string or slices with an arbitrary name.
The right thing to do is look at the go kind (for these types and eventually also map and chan). Hardcoded formatters seems like the only way to do this.
I also considered trying to represent this as a base type or typedef, but those both seemed complicated and confusing since they're not things the language itself supports.

source/Plugins/Language/Go/GoFormatterFunctions.cpp
10 ↗(On Diff #37800)

See source/Plugins/Language/Go/GoLanguage.h below. At least this style is consistent across LLDB headers.

ribrdb updated this revision to Diff 37898.Oct 20 2015, 11:11 AM

Did you format code with Clang-format? Spaces/no spaces between function names and arguments are inconsistent.

source/Plugins/Language/Go/GoFormatterFunctions.h
29 ↗(On Diff #37898)

This header belongs to Other libraries and framework includes.

source/Plugins/Language/Go/GoLanguage.cpp
25 ↗(On Diff #37898)

This and next include belong to C++ Includes.

ribrdb updated this revision to Diff 37900.Oct 20 2015, 11:25 AM
ribrdb removed rL LLVM as the repository for this revision.

Oops, I ran clang-format with the wrong arguments. Ran it again.

Is this ready to submit?

granata.enrico accepted this revision.Oct 29 2015, 2:25 PM
granata.enrico edited edge metadata.

I think it's OK to land this, yes

I would still like to think about ways that we can avoid using hardcoded formatters here, but it seems non-trivial and I don't know enough about Go to suggest an obvious solution

This revision is now accepted and ready to land.Oct 29 2015, 2:25 PM
This revision was automatically updated to reflect the committed changes.