[PATCH] D116113: Add LLDB synthetic child and summary scripts for llvm::SmallVector, llvm::Optional, llvm::ErrorOr and llvm::Expected.
wallace added a comment. thanks for doing this. Just a few minor comments and i think this is good to go Comment at: clang/utils/ClangDataFormat.py:215-218 +if self.hasVal: +result = self.value.GetIndexOfChildWithName(name) +return None if result == 4294967295 else result +return None it's safe to return 0 here, as it'll be only invoked when there's a value, so there's a single child with index 0 Comment at: clang/utils/ClangDataFormat.py:221-223 +if self.hasVal: +return self.value.GetChildAtIndex(index) +return None wouldn' just return self.value be enough? index is always going to be 0 if called Comment at: clang/utils/ClangDataFormat.py:279 + +class Expected(object): +def __init__(self, valobj, internal_dict): god bless you Comment at: clang/utils/ClangDataFormat.py:331-335 +# We compute the right value to use as the lldb.SBValue to use in +# self.value in the update() method so we can just use that object to +# get our asnwers +result = self.value.GetIndexOfChildWithName(name) +return None if result == 4294967295 else result you can just return 0 Comment at: clang/utils/ClangDataFormat.py:409-410 +# get our asnwers +result = self.value.GetIndexOfChildWithName(name) +return None if result == 4294967295 else result + ditto Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116113/new/ https://reviews.llvm.org/D116113 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88103: [JSON] Add error reporting facility, used in fromJSON and ObjectMapper.
wallace added a comment. I like this a lot!! Amazing work. I think it'll be useful for my Trace patch. I'll do a corresponding refactor using probably this new functionality. I find the diff straightforward to follow. Unless anyone has objections, I'm fine with the patch at it is. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88103/new/ https://reviews.llvm.org/D88103 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D83731: Add Debug Info Size to Symbol Status
wallace accepted this revision. wallace added inline comments. Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:65 +@skipIfWindows +@ skipUnlessDarwin +@skipIfRemote remove this whitespace Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83731/new/ https://reviews.llvm.org/D83731 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D83731: Add Debug Info Size to Symbol Status
wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.
The logic looks very good, just some final comments and the code will be high
quality
Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:66
+@skipIfRemote
+def test_module_event(self):
+# Mac or linux.
With you current Makefile, this test will fail on Linux, as the Makefile will
expect a .dsym file to be created. Simply put @ skipUnlessDarwin back here, and
add this comment
#TODO: Update the Makefile so that this test runs on Linux
Once this is committed, I'll work on make this test pass on Linux
Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:67-73
+# Mac or linux.
+
+# On mac, if we load a.out as our symbol file, we will use DWARF with
.o files and we will
+# have debug symbols, but we won't see any debug info size because all
of the DWARF
+# sections are in .o files.
+
+# On other platforms, we expect a.out to have debug info, so we will
expect a size.
The common way to write function comments is with ''', like this
'''
Mac or linux.
On mac, if we load a.out as our symbol file, we will use DWARF with .o
files and we will
have debug symbols, but we won't see any debug info size because all of the
DWARF
sections are in .o files.
On other platforms, we expect a.out to have debug info, so we will expect a
size.
'''
That way you don't need to type that many #
Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:81-84
+# Darwin only test with dSYM file.
+
+# On mac, if we load a.out.dSYM as our symbol file, we will have debug
symbols and we
+# will have DWARF sections added to the module, so we will expect a
size.
same here about the comment
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:333
+ llvm::StringRef section_name(section.GetName());
+ if (section_name.startswith(".debug") || section_name.startswith("__debug")
+ || section_name.startswith(".apple") ||
section_name.startswith("__apple"))
apply the format change it suggests, i.e start the second line with ||
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:353
+
+static std::string ConvertDebugInfoSize(uint64_t debug_info) {
+ char debug_info_size[32];
ConvertDebugInfoSizeToString is a better name
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:354-368
+ char debug_info_size[32];
+ if (debug_info < 1024) {
+snprintf(debug_info_size, sizeof(debug_info_size), " (%" PRIu64 "B)",
+ debug_info);
+ } else if (debug_info < 1024 * 1024) {
+double kb = double(debug_info) / 1024.0;
+snprintf(debug_info_size, sizeof(debug_info_size), " (%.1fKB)", kb);
a more modern way to implement this is
#include
#include
...
std::ostringstream oss;
oss << "(";
oss << std::fixed << std::setprecision(1);
if (debug_info < 1024) {
oss << debug_info << "B";
} else if (debug_info < 1024 * 1024) {
double kb = double(debug_info) / 1024.0;
oss << kb << "KB";
} else if (debug_info < 1024 * 1024 * 1024) {
double mb = double(debug_info) / (1024.0 * 1024.0);
oss << mb << "MB";
} else {
double gb = double(debug_info) / (1024.0 * 1024.0 * 1024.0);
oss << gb << "GB";;
}
oss << ")";
return oss.str();
It's actually safer, as you don't need to specify the array size of your
debug_info_size_buffer
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:385-386
+if (debug_info > 0) {
+ std::string debug_info_size = ConvertDebugInfoSize(debug_info);
+ symbol_str = symbol_str + debug_info_size;
+}
symbol_str += ConvertDebugInfoSizeToString(debug_info);
is more concise
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83731/new/
https://reviews.llvm.org/D83731
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D83731: Add Debug Info Size to Symbol Status
wallace added a comment. @clayborg, the tests will fail on linux because the Makefile is expecting a .dsym file. I think it's okay if she does it just for Darwin and then I update the test to cover also linux, as I have my linux already well set up. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83731/new/ https://reviews.llvm.org/D83731 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D83731: Add Debug Info Size to Symbol Status
wallace added inline comments.
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:333-335
+ if (section_name.startswith(".debug") || section_name.startswith("__debug"))
+debug_info_size += section.GetFileByteSize();
+ if (section_name.startswith(".apple") || section_name.startswith("__apple"))
you can merge these two ifs
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83731/new/
https://reviews.llvm.org/D83731
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
