Store sizeof and friends as numbers instead of comment.
ClosedPublic

Authored by michalsrb on Jul 30 2018, 9:45 AM.

Details

Summary

This way they can be used by other plugins, for example to visualize padding
in structures. They also take a bit less space in duchain in this form.
It is not longer necessary to exclude them from the json test run, since
they are not part of the comment.

The end result is now the same - the values are shown as text in tooltip.

Test Plan

Verify that message similar to "size: 96 Bytes; aligned to: 8 Bytes" is
shown in tooltip for classes and class members.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
michalsrb created this revision.Jul 30 2018, 9:45 AM
Restricted Application added a project: KDevelop. · View Herald TranscriptJul 30 2018, 9:45 AM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
michalsrb requested review of this revision.Jul 30 2018, 9:45 AM

The open question is whether the values should be stored as int64_t. I have chosen it because that is what the clang querying functions return. But in typical scenarios the values will be lot smaller - especially the align of.

So using int32_t or even int16_t and falling back to -1 (unknown) in case clang reported bigger number is an option.

Nice. Having this information better structured and accessible is definitely a good thing!

kdevplatform/language/duchain/classmemberdeclaration.cpp
32–34

Maybe you should document somewhere that -1 means that no information is available.

kdevplatform/language/duchain/classmemberdeclarationdata.h
40

If we want to reduce the storage space required, we could use that alignment is always a power of two and only store the exponent. Since int64_t can store values up to 2^63-1, we need 6 bits for the exponent.

michalsrb updated this revision to Diff 39334.Aug 9 2018, 8:26 AM

Document the -1 value and store alignOf as exponent.

I also reordered the fields in ClassMemberDeclarationData to reduce wasted
space on padding.

aaronpuchert accepted this revision.Aug 10 2018, 12:08 AM

Otherwise it's fine.

kdevplatform/language/duchain/classmemberdeclaration.cpp
191

I guess you meant to return here.

193

Make this unsigned to avoid signed -> unsigned conversion when storing the value.

194–197

Can be slightly simplified, because we know here that alignedTo != 0:

while (alignedTo >>= 1)
  alignOfExponent++;
This revision is now accepted and ready to land.Aug 10 2018, 12:08 AM

Do you have commit access or should I commit this for you?

michalsrb updated this revision to Diff 39584.Aug 13 2018, 11:56 AM

Fixes in ClassMemberDeclaration::setAlignOf.

michalsrb marked 5 inline comments as done.Aug 13 2018, 11:58 AM

Thank you for the review. I did the changes to ClassMemberDeclaration::setAlignOf.

I do not have commit access, please commit it.

brauch added a subscriber: brauch.Aug 13 2018, 12:08 PM
brauch added inline comments.
kdevplatform/language/duchain/classmemberdeclaration.h
75

this comment is wrong, it returns the size, not the alignment

77

Why is this a 64-bit integer? I.e. why is 32-bit not sufficient?

michalsrb added inline comments.Aug 13 2018, 3:20 PM
kdevplatform/language/duchain/classmemberdeclaration.h
75

copy paste mistake, I will fix that

77

I chose that because that is what clang_Type_getSizeOf returns. You can declare something like char c[0x100000000] and it will display the size properly.

But I expect that almost no real code will have sizes or offsets this big, so we could use 32-bit integer instead and fallback to -1 for the extreme cases. Do you think we should?

aaronpuchert added inline comments.Aug 13 2018, 7:33 PM
kdevplatform/language/duchain/classmemberdeclaration.h
77

For practical purposes, sizes >= 2^31 are probably indeed irrelevant. However, I think we should distinguish between no size information (which will happen for template classes, for example) and the size being to big. (Then we could show "size >= 2^31 bytes" or something similar in the UI.)

We could use -2 for that case (we have all the negative numbers available), but we should introduce symbolic constants for both -1 and -2 then.

Ok, fine with me, I don't care much about the type. I just stumbled upon the comment and then noticed the type as well. Thanks for the patch, all fine from my side :)

aaronpuchert accepted this revision.Aug 13 2018, 7:49 PM

Ok, I take it that space optimizations could also be done later if necessary. I will merge it then after correcting the comment.

This revision was automatically updated to reflect the committed changes.