[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

dblaikie added a comment. https://github.com/llvm/llvm-project/issues/63169 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124221/new/ https://reviews.llvm.org/D124221 ___ cfe-commits mailing list cfe-com

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

erichkeane added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2441 +well as aggregate structures, ``void*`` (printed with ``%p``), and ``const +char*`` (printed with ``%s``). A ``*%p`` specifier will be used for a field +that Clang doesn't know how to format, an

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

yihanaa added a comment. This is unfortunately true, unless we generate some code that calculates the length of the string at runtime. I find it difficult to draw the line between defining this built-in functionality and developers developing printf-like functionality, if built-in does too much

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

erichkeane added a comment. I was thinking of that, but I don't have a good way using just format strings to get us to print the '...' after. I think if someone wants that part of the feature, they'll have to wrap printf/etc in a wrapper that does that. Repository: rG LLVM Github Monorepo

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

yihanaa added a comment. I found LLDB have this feature. LLDB uses "..." to indicate that only part of the string is now printed, and it also have a command to modify length limit. This approach is similar to the idea of @erichkeane, can we do the same？what do you all think? typedef signed

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

erichkeane added a comment. In D124221#3494000 , @aaron.ballman wrote: > In D124221#3493792 , @erichkeane > wrote: > >> FWIW, I'm in favor of the patch as it sits. >> >> As a followup: So I was thinking about th

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

aaron.ballman added a comment. In D124221#3493792 , @erichkeane wrote: > FWIW, I'm in favor of the patch as it sits. > > As a followup: So I was thinking about the "%s" specifier for string types. > Assuming char-ptr types are all strings is a LITTLE d

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

erichkeane accepted this revision. erichkeane added a comment. FWIW, I'm in favor of the patch as it sits. As a followup: So I was thinking about the "%s" specifier for string types. Assuming char-ptr types are all strings is a LITTLE dangerous, but more so the way we're doing it. Its a shame

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from a minor nit, I think this LGTM. Thank you for working on this! Comment at: clang/lib/Sema/SemaChecking.cpp:476 +analyze_printf::PrintfSpecifier S

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

yihanaa added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:433 + + llvm::StringRef getFormatSpecifier(QualType T) { +if (auto *BT = T->getAs()) { yihanaa wrote: > erichkeane wrote: > > rsmith wrote: > > > yihanaa wrote: > > > > rsmith wrote:

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

yihanaa added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:433 + + llvm::StringRef getFormatSpecifier(QualType T) { +if (auto *BT = T->getAs()) { erichkeane wrote: > rsmith wrote: > > yihanaa wrote: > > > rsmith wrote: > > > > rsmith wrote: >

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:433 + + llvm::StringRef getFormatSpecifier(QualType T) { +if (auto *BT = T->getAs()) { rsmith wrote: > yihanaa wrote: > > rsmith wrote: > > > rsmith wrote: > > > > aaron.ballman wr

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:433 + + llvm::StringRef getFormatSpecifier(QualType T) { +if (auto *BT = T->getAs()) { yihanaa wrote: > rsmith wrote: > > rsmith wrote: > > > aaron.ballman wrote: > > > > yihanaa wrote

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

yihanaa added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:433 + + llvm::StringRef getFormatSpecifier(QualType T) { +if (auto *BT = T->getAs()) { rsmith wrote: > rsmith wrote: > > aaron.ballman wrote: > > > yihanaa wrote: > > > > I think this

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:433 + + llvm::StringRef getFormatSpecifier(QualType T) { +if (auto *BT = T->getAs()) { rsmith wrote: > aaron.ballman wrote: > > yihanaa wrote: > > > I think this is better maintained i

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

erichkeane added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2442 + +This builtin does not return a value. + rsmith wrote: > erichkeane wrote: > > I don't know if anyone would be using this value, but I wonder if there is > > value to making t

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8470-8473 +def err_expected_struct_pointer_argument : Error< + "expected pointer to struct as %ordinal0 argument to %1, found %2">; +def err_expected_callable_argument : Error< + "

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2442 + +This builtin does not return a value. + erichkeane wrote: > I don't know if anyone would be using this value, but I wonder if there is > value to making this a 'sum' of the resul

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith updated this revision to Diff 426525. rsmith marked 3 inline comments as done. rsmith added a comment. - Add requested assert. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124221/new/ https://reviews.llvm.org/D124221 Files: clang/docs/La

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith updated this revision to Diff 426520. rsmith added a comment. - Use printing policy more, and turn off anonymous tag locations. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124221/new/ https://reviews.llvm.org/D124221 Files: clang/docs/L

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith updated this revision to Diff 426517. rsmith marked 5 inline comments as done. rsmith added a comment. - Respond to review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124221/new/ https://reviews.llvm.org/D124221 Files: clang/d

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

yihanaa added a comment. Do we need to do argument promotion here，ahh...maybe i understood wrong Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124221/new/ https://reviews.llvm.org/D124221 ___ cfe-commits

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

yihanaa added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:573 + // We don't know how to print this field. Print out its address + // with a format specifier that a smart tool will be able to + // recognize and treat specially. -

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

yihanaa added a comment. This implementation looks good to me😁 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124221/new/ https://reviews.llvm.org/D124221 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

erichkeane added a comment. This is a pretty nice seeming interface that I think does a fairly good job at maintaining backwards compat while improving the functionality. A few questions/comments. Comment at: clang/docs/LanguageExtensions.rst:2442 + +This builtin does not ret

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

aaron.ballman added a comment. Thanks for working on this, I like the direction it's going! Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8470-8473 +def err_expected_struct_pointer_argument : Error< + "expected pointer to struct as %ordinal0 argument to %1, foun

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

yihanaa added inline comments. Comment at: clang/docs/ReleaseNotes.rst:174 - Improve the dump format, dump both bitwidth(if its a bitfield) and field value. - - Remove anonymous tag locations. - - Beautify dump format, add indent for nested struct and struct members. + - R

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

yihanaa added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:427 +llvm::SmallString<32> Indent; +Indent.resize(Depth * 4, ' '); +return getStringLiteral(Indent); What do you think of PrintingPolicy.Indentation here? Co

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith updated this revision to Diff 426349. rsmith added a comment. - Update release notes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124221/new/ https://reviews.llvm.org/D124221 Files: clang/docs/LanguageExtensions.rst clang/docs/ReleaseN

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith updated this revision to Diff 426348. rsmith added a comment. - Minor doc correction Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124221/new/ https://reviews.llvm.org/D124221 Files: clang/docs/LanguageExtensions.rst clang/docs/ReleaseN

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith updated this revision to Diff 426347. rsmith edited the summary of this revision. rsmith added a comment. - Fix example in documentation: the dump is terminated by a newline. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124221/new/ https://

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith added a comment. In D124221#3469251 , @rsmith wrote: > I think we can address most of my concerns with `__builtin_dump_struct` > without a new builtin, by incorporating things from this implementation Done; this patch now reimplements `__builtin_