This is an archive of the discontinued LLVM Phabricator instance.

Fix incorrect formatting of DataRefImpl members in operator<< function
ClosedPublic

Authored by davidb on Jan 20 2017, 9:31 AM.

Details

Summary

Changed format specifiers to use format macro constant for pointer type.
Moved width part of format specifier in the correct place for formatting members a and b.

Added a unit test to confirm the output.

Diff Detail

Event Timeline

davidb created this revision.Jan 20 2017, 9:31 AM
This revision is now accepted and ready to land.Feb 9 2017, 8:18 AM
filcab added a subscriber: filcab.Feb 9 2017, 8:46 AM
filcab added inline comments.
include/llvm/Object/SymbolicFile.h
35

Please don't add whitespace.

38

Shouldn't it be "...%" PRIxPTR?
Why change the way it dumps pointers, though? Even for uint32_t, we're dumping as 8B. Why not always dump as 8B and be done with it?

filcab added inline comments.Feb 9 2017, 8:47 AM
include/llvm/Object/SymbolicFile.h
38

Since you're changing this line anyway, can you clang-format it?

davidb marked 2 inline comments as done.Feb 9 2017, 10:07 AM
davidb added inline comments.
include/llvm/Object/SymbolicFile.h
38

Why change the way it dumps pointers, though? Even for uint32_t, we're dumping as 8B. Why not always dump as 8B and be done with it?

Sorry, not entirely sure I understand what you are asking. I changed it because it was unable to dump a 64-bit pointer.

I intentionally tried to keep the format as close as the original intent appeared to be (minimum width of 8 characters to represent the full 32-bit field). In the case that p > 32-bit value, then the character length will of course expand....

filcab added inline comments.Feb 9 2017, 1:32 PM
include/llvm/Object/SymbolicFile.h
38

Nevermind my 8B stuff. I was thinking in terms of bytes, not in terms of nibbles (which will be the width here).

This revision was automatically updated to reflect the committed changes.
davidb marked 3 inline comments as done.