This is an archive of the discontinued LLVM Phabricator instance.

DebugInfo: use uin32_t for alignment
ClosedPublic

Authored by vleschuk on Oct 14 2016, 7:38 AM.

Diff Detail

Event Timeline

vleschuk updated this revision to Diff 74685.Oct 14 2016, 7:38 AM
vleschuk retitled this revision from to DebugInfo: use DIAlignment type..
vleschuk updated this object.
vleschuk added a subscriber: cfe-commits.
aprantl added inline comments.Oct 14 2016, 9:34 AM
include/clang/AST/ASTContext.h
83 ↗(On Diff #74685)

I'm not sure we want to use a debug info type inside the AST. I think we only want to use them in CGDebugInfo.cpp.

aprantl added a reviewer: rnk.Oct 14 2016, 9:37 AM
vleschuk marked an inline comment as done.Oct 17 2016, 4:36 AM
vleschuk added inline comments.
include/clang/AST/ASTContext.h
83 ↗(On Diff #74685)

We use TypeInfo and related functions heavily in CGDebugInfo.cpp, leaving this field as "unsigned" will make us to perform conversions from "unsigned" to llvm::DIAlignment, I think having this changed will result in simpler code.

vleschuk marked an inline comment as done.Oct 17 2016, 9:48 AM
vleschuk added inline comments.
include/clang/AST/ASTContext.h
83 ↗(On Diff #74685)

@dblaikie wrote:

It'd still be a layering violation to have AST depend on debug info types/concepts. If it makes sense to change it to uint32_t in AST, that'd probably be reasonable.

We are discussing the possibility of changing this to "unsigned" in https://reviews.llvm.org/D25620, after we come to consensus I'll update both patches. Thanks for the advice, I agree that using DebugInfo types in AST was a bad idea.

vleschuk updated this revision to Diff 74899.Oct 17 2016, 2:01 PM
  • Use uint32_t directly for alignment instead of creating typedef ofr it
  • Get rid of DebugInfo dependency in AST
vleschuk retitled this revision from DebugInfo: use DIAlignment type. to DebugInfo: use uin32_t for alignment.Oct 17 2016, 2:01 PM
vleschuk updated this object.
vleschuk marked 2 inline comments as done.Oct 18 2016, 1:20 PM

Can anyone take a look at this please? =)

aprantl edited edge metadata.Oct 18 2016, 3:40 PM

This patch is conflating two set of changes:
(1) NFC: rename all occurrences of unsigned for alignment purposes in the frontend with uint32_t
(2) shrink all debug-info-related alignment variables from uint64_t -> unint32_t.

I think this patch should only be doing the changes in (2).

vleschuk updated this revision to Diff 75143.Oct 19 2016, 6:55 AM
vleschuk edited edge metadata.
  • Removed all changes non-related to CGDebugInfo: will post them in separate NFC patch which will switch all unsigned alignment entries to uint32_t.

This patch is conflating two set of changes:
(1) NFC: rename all occurrences of unsigned for alignment purposes in the frontend with uint32_t
(2) shrink all debug-info-related alignment variables from uint64_t -> unint32_t.

I think this patch should only be doing the changes in (2).

Agreed. Update this one. The (1) will come as separate NFC review along with corresponding LLVM patch.

aprantl accepted this revision.Oct 19 2016, 8:21 AM
aprantl edited edge metadata.

Thanks, this LGTM!

This revision is now accepted and ready to land.Oct 19 2016, 8:21 AM
This revision was automatically updated to reflect the committed changes.