This is an archive of the discontinued LLVM Phabricator instance.

[builtins] __builtin_dump_struct : added more types format
ClosedPublic

Authored by paulsemel on Apr 13 2018, 2:53 AM.

Details

Reviewers
aaron.ballman
Summary

There was missing some basic types format (like uint8_t..). This patch is meant to print them the correct way.

Diff Detail

Repository
rC Clang

Event Timeline

paulsemel created this revision.Apr 13 2018, 2:53 AM

Tests?

I can do this. But currently, I am not testing the formats in the tests..

aaron.ballman requested changes to this revision.Apr 13 2018, 5:42 AM

Tests?

I can do this. But currently, I am not testing the formats in the tests..

Now might be a good time to test that, because this patch almost added a bug by missing the length modifiers. Also, all patches should come with some tests to demonstrate the behavioral differences from the current trunk.

lib/CodeGen/CGBuiltin.cpp
954–955

Can you keep the signed/unsigned ordering?

Also, I think these should have the length modifiers added (hh for char).

Finally: ShortTy and UnsignedShortTy were already handled (see lines 962-963 in this patch), so I don't think those need to be added.

This revision now requires changes to proceed.Apr 13 2018, 5:42 AM

Tests?

I can do this. But currently, I am not testing the formats in the tests..

Now might be a good time to test that, because this patch almost added a bug by missing the length modifiers. Also, all patches should come with some tests to demonstrate the behavioral differences from the current trunk.

Sure. I will do another review for the existing format testing, and then go back to this one to finish the type handling !

paulsemel updated this revision to Diff 142750.Apr 17 2018, 3:52 AM

Removed the two existing types.
Added the correct format sizing.
Added tests for those formats.
These version is now relying on the previous patch : https://reviews.llvm.org/D45665

aaron.ballman accepted this revision.Apr 17 2018, 7:00 AM

LGTM! I will go ahead and commit on your behalf. Btw, if you're thinking of making additional patches, now might be a good time to ask for commit privileges (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access).

This revision is now accepted and ready to land.Apr 17 2018, 7:00 AM
aaron.ballman closed this revision.Apr 17 2018, 7:03 AM

Committed in r330188, thank you for the patch!

LGTM! I will go ahead and commit on your behalf. Btw, if you're thinking of making additional patches, now might be a good time to ask for commit privileges (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access).

Thanks ! I actually really want to continue improving the builtin, so yes, I will make additional patches ! I just asked for accesses today :)