This is an archive of the discontinued LLVM Phabricator instance.

[NFC, Refactor] Convert TypeSpecifierWidth from Specifiers.h to a scoped enum
ClosedPublic

Authored by tschuett on Nov 13 2020, 2:58 AM.

Details

Summary

ninja clang clangd

Diff Detail

Event Timeline

tschuett created this revision.Nov 13 2020, 2:58 AM
tschuett requested review of this revision.Nov 13 2020, 2:58 AM
tschuett edited the summary of this revision. (Show Details)
aaron.ballman added inline comments.Nov 13 2020, 6:17 AM
clang/include/clang/Basic/Specifiers.h
111

This changes the size of the structure. It used to be a two byte struct (needing 11 bits with 5 bits of padding) and now I think it will wind up being a three byte struct (9 bits for bit-fields, 6 bits of padding, and then 8 bits for the enum). Do you know how this impacts memory footprint or build performance?

clang/include/clang/Sema/DeclSpec.h
338

This change also changes the size of the structure, but further, it breaks the allocation unit for the bit-fields because TypeSpecComplex can no longer pack together with SCS_extern_in_linkage_spec due to the intervening non-bit-field.

tschuett updated this revision to Diff 305137.Nov 13 2020, 7:11 AM
  • reverted changes to structs
  • added static_casts
tschuett marked 2 inline comments as done.Nov 13 2020, 7:12 AM
aaron.ballman added inline comments.Nov 13 2020, 7:44 AM
clang/include/clang/Basic/Specifiers.h
40

I don't think there's a lot of benefit to fixing the underlying type to unsigned char now that we're not using the type in a structure -- should this be left off (so it's int implicitly)?

109

How about: /*TypeSpecifierWidth*/ unsigned Width: 2;?

clang/include/clang/Sema/DeclSpec.h
338

How about: /*TypeSpecifierWidth*/ unsigned Width: 2;?

clang/lib/Sema/DeclSpec.cpp
690

Do we maybe want to use getTypeSpecWidth() instead of all of the static_cast<>s in this file?

tschuett updated this revision to Diff 305161.Nov 13 2020, 8:20 AM
  • added comments to members
  • removed underlying type
  • replace static_casts with getTypeSpecWidth()
tschuett marked 3 inline comments as done.Nov 13 2020, 8:21 AM
tschuett marked an inline comment as done.
tschuett added inline comments.
clang/lib/Sema/DeclSpec.cpp
690

Thanks!

aaron.ballman accepted this revision.Nov 13 2020, 9:48 AM

LGTM aside from a place where I think we can remove a cast.

clang/lib/Sema/DeclSpec.cpp
1100

Should this one be TypeSpecWidth since it's already an unsigned?

This revision is now accepted and ready to land.Nov 13 2020, 9:48 AM
tschuett marked an inline comment as done.Nov 13 2020, 10:02 AM

Could you please commit this for me as I have no privileges. "Thorsten Schuett <schuett@gmail.com>". Thanks.

clang/lib/Sema/DeclSpec.cpp
1100

getTypeSpecWidth() returns a TypeSpecWidth and writttenBS.Width is an unsigned?

tschuett added inline comments.Nov 13 2020, 10:30 AM
clang/lib/Sema/DeclSpec.cpp
1100

Yes, I will change it.

tschuett updated this revision to Diff 305210.EditedNov 13 2020, 10:33 AM

replaced static_cast<unsigned>(getTypeSpecWidth()) by TypeSpecWidth

tschuett marked an inline comment as done.Nov 13 2020, 10:33 AM
faisalv closed this revision.Nov 15 2020, 9:19 AM

Committed here: https://github.com/llvm/llvm-project/commit/e6aa06545b123292be283af7c414daead23cf9ab .

Thanks Thorsten - please keep these patches coming :)

Committed here: https://github.com/llvm/llvm-project/commit/e6aa06545b123292be283af7c414daead23cf9ab .

Thanks Thorsten - please keep these patches coming :)

Thank you for landing this, Faisal, and thank you for the patch Thorsten!