Page MenuHomePhabricator

Fix libc++ vector<bool> data formatter (bug #32553)
ClosedPublic

Authored by labath on Apr 10 2017, 4:03 AM.

Details

Summary

The iteration list through the available data formatters was undefined,
which meant that the vector<bool> formatter kicked in only in cases
where it happened to be queried before the general vector formatter. To
fix this, I merge the two data formatter entries into one, and select
which implementation to use in the factory function.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Apr 10 2017, 4:03 AM
labath updated this revision to Diff 94667.Apr 10 2017, 7:15 AM

Remove the vector<bool> summary provider as well (picked up from Tamas's version of the patch).

tberghammer edited edge metadata.Apr 10 2017, 10:47 AM

The previous version of the data formatter was triggering for std::vector<std::allocator<bool>> as well. Jason, do you know why was it the case? Do we need that functionality because of a broken compiler version or can it be removed?

Note: I added a few comments about the data formatter itself but feel free to ignore them if you want to keep this change small.

source/Plugins/Language/CPlusPlus/LibCxxVector.cpp
52–53 ↗(On Diff #94667)

Not needed

201–228 ↗(On Diff #94667)

This switch seems silly. You can just replace it with "mask = (1 << bit_index)"

233–234 ↗(On Diff #94667)

I think the formatting here makes the code pretty hard to read

301–304 ↗(On Diff #94667)

Is there a reason you are not using ValueObject::GetCompilerType()?

labath marked 3 inline comments as done.Apr 11 2017, 5:50 AM
labath added inline comments.
source/Plugins/Language/CPlusPlus/LibCxxVector.cpp
301–304 ↗(On Diff #94667)

Because I did not know it exists. :)

labath updated this revision to Diff 94807.Apr 11 2017, 5:57 AM

Address review comments.

jasonmolenda edited edge metadata.Apr 11 2017, 10:08 PM

This all looks good to me, thanks for doing this Pavel. Tamas asked in an earlier comment, "The previous version of the data formatter was triggering for std::vector<std::allocator<bool>> as well. Jason, do you know why was it the case? Do we need that functionality because of a broken compiler version or can it be removed?"

I have no idea why that was there. I could dig around in the svn commit history to try to figure it out, but it may have been unnecessary to begin with. If in doubt, we can drop it and put it back in if we hear of any problems.

This revision was automatically updated to reflect the committed changes.