Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Unit tests: pass. 61771 tests passed, 0 failed and 780 were skipped.
clang-tidy: fail. Please fix clang-tidy findings.
clang-format: pass.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
debuginfo-tests/llvm-prettyprinters/gdb/prettyprinters.cpp | ||
---|---|---|
64 | Indeed, don't need to capture globals. | |
llvm/utils/gdb-scripts/prettyprinters.py | ||
371–374 | I changed it to inspect the template parameter, instead of blindly trying both. It's not possible to check the 'Prev' code path without rebuilding LLVM with different #defines. |
Unit tests: pass. 62221 tests passed, 0 failed and 816 were skipped.
clang-tidy: fail. clang-tidy found 1 errors and 0 warnings. 0 of them are added as review comments below (why?).
clang-format: pass.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.
debuginfo-tests/llvm-prettyprinters/gdb/prettyprinters.cpp | ||
---|---|---|
64 | Ping on this - please remove the capture (remove the "&" in the "[&]") since it doesn't capture anything. (global variables are accessible directly, without capturing them) | |
llvm/utils/gdb-scripts/prettyprinters.py | ||
380–381 | Drop the else-after-return, per LLVM style: if x: return y() return z() | |
399 | Do you think the make_printer usage is sufficiently more readable than having a standalone pretty printer type (like IlistPrinter below)? I think there might be some value in just always writing these as standalone pretty printer classes rather than having some of them as factories and some as classes. |
Unit tests: pass. 62221 tests passed, 0 failed and 816 were skipped.
clang-tidy: pass.
clang-format: pass.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.
Unit tests: pass. 62221 tests passed, 0 failed and 816 were skipped.
clang-tidy: pass.
clang-format: pass.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.
debuginfo-tests/llvm-prettyprinters/gdb/prettyprinters.cpp | ||
---|---|---|
64 | Sorry, I think the update was in flight while you were reviewing. | |
llvm/utils/gdb-scripts/prettyprinters.py | ||
399 | I changed it to a class, which makes sense because initialization should not fail. But for what it's worth, I don't really see the 'some value'. I think it was perfectly readable and concise before. |
debuginfo-tests/llvm-prettyprinters/gdb/prettyprinters.gdb | ||
---|---|---|
48–129 | Could you check these with the surrounding syntax too to give a better sense of how the printing would look to the user? (I guess they'll print in {}, is there a type prefix like with the other pretty printers (eg: "llvm::ArrayRef of", etc)? |
Thanks for the details! I'd be /OK/ with this, but it doesn't seem ideal to be printing the next/prev when printing the container? Do you think it's worth/there's any nice way to avoid that significant redundancy/verbosity in the printing & just print the payload?
debuginfo-tests/llvm-prettyprinters/gdb/prettyprinters.gdb | ||
---|---|---|
48–129 | Done. No, there is no prefix because the printer has no to_string method. That's intentional because gdb does not print the type for ordinary values either. There is ptype for that. CLion watches show the result of both print and ptype. I should probably go back and remove it from the PointerIntPair as well, and add individual printer classes while I'm at it. |
Unit tests: pass. 62221 tests passed, 0 failed and 816 were skipped.
clang-tidy: pass.
clang-format: pass.
Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml
Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.
Well, it's an intrusive list. I'm not aware of any way to hide the ilist_node base class when printing the list, that would be the responsibility of the derived class' printer (which probably wouldn't want to do that, in case someone prints an individual node).
children can only return a list of gdb.Value, and gdb picks the printers for those internally. And to make things a little more complicated, like in the test here, there can be multiple ilist_node base classes and you would only want to silence the one with the corresponding tag.
(looks like this capture should be removed, according to the lint check)