Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
source/Plugins/Language/Go/GoFormatterFunctions.cpp | ||
---|---|---|
104 | 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 | 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 | 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 | Please add include section comments. Same for other sections. | |
98 | Please place destructor near constructor. | |
source/Plugins/Language/Go/GoLanguage.cpp | ||
49 | Please use consistent spacing between methods. | |
source/Plugins/Language/Go/GoLanguage.h | ||
31 | Please use override instead of virtual. | |
34 | Please place constructor before destructor. | |
65 | virtual is not needed. Please run clang-tidy modernize-use-override. Same for GetPluginVersion(). |
source/Plugins/Language/Go/GoFormatterFunctions.h | ||
---|---|---|
1 | Such comments should be 80 characters long. | |
15 | Please add headers sections comments. Same for other sections. Clang headers should be before LLDB. | |
source/Plugins/Language/Go/GoLanguage.cpp | ||
11 | 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 | One space should be enough. |
source/Plugins/Language/Go/GoFormatterFunctions.cpp | ||
---|---|---|
10 | 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 | Normal string and slice types can be recognized by name. |
source/Plugins/Language/Go/GoFormatterFunctions.cpp | ||
---|---|---|
10 | 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 | ||
---|---|---|
30 | This header belongs to Other libraries and framework includes. | |
source/Plugins/Language/Go/GoLanguage.cpp | ||
26 | 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
Such comments should be 80 characters long.