Page MenuHomePhabricator

Fix assertion in ClangASTContext
ClosedPublic

Authored by labath on Nov 29 2017, 10:41 AM.

Details

Summary

llvm::APSInt(0) asserts because it creates an int with bit-width 0 and
not (as I thought) a value 0.

Theoretically it should be sufficient to change this to APSInt(1), as
the intention there was that the value of the first argument should be
ignored if the type is invalid, but that would look dodgy.

Instead, I use llvm::Optional to denote an invalid value and use a
special struct instead of a std::pair, to reduce typing and increase
clarity.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Nov 29 2017, 10:41 AM
clayborg accepted this revision.Nov 29 2017, 10:55 AM

Few nits, but nothing that would hold up the patch. Looks good.

include/lldb/Symbol/CompilerType.h
294 ↗(On Diff #124781)

Can't you just define the type right here?

source/Plugins/Language/CPlusPlus/LibCxxBitset.cpp
62–63 ↗(On Diff #124781)

Should we combine these two lines?

if (auto arg = m_backend.GetCompilerType().GetIntegralTemplateArgument(0))
This revision is now accepted and ready to land.Nov 29 2017, 10:55 AM
labath marked an inline comment as done.Nov 30 2017, 1:59 AM
labath added inline comments.
include/lldb/Symbol/CompilerType.h
294 ↗(On Diff #124781)

Unfortunately, I can't because it contains a CompilerType member, which means CompilerType has to be a complete type, and this only happens after we reach the end of the class definition.

However, I may have gone a bit overboard with the nesting of this type inside CompilerType. I could make this just a standalone class if you think that will make things more understandable (a forward declaration will still be needed though).

This revision was automatically updated to reflect the committed changes.