Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
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(). |
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.
source/Plugins/Language/Go/GoFormatterFunctions.cpp | ||
---|---|---|
107 ↗ | (On Diff #37800) | One space should be enough. |
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. |
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. |
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. |
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