This is an archive of the discontinued LLVM Phabricator instance.

TrailingObjects::FixedSizeStorage constexpr fixes + tests
ClosedPublic

Authored by hubert.reinterpretcast on Jul 22 2016, 2:57 AM.

Details

Summary

This change fixes issues with LLVM_CONSTEXPR functions and TrailingObjects::FixedSizeStorage. In particular, some of the functions marked LLVM_CONSTEXPR used by FixedSizeStorage were not implemented such that they evaluate successfully as part of a constant expression despite constant arguments.

This change also implements a more traditional template-meta path to accommodate MSVC, and adds unit tests for FixedSizeStorage.

Drive-by fix: the access control for member of TrailingObjectsImpl is tightened.

Diff Detail

Event Timeline

hubert.reinterpretcast retitled this revision from to TrailingObjects::FixedSizeStorage constexpr fixes.
hubert.reinterpretcast updated this object.
hubert.reinterpretcast added a subscriber: cfe-commits.
aaron.ballman edited edge metadata.Jul 28 2016, 7:21 AM

I don't suppose there's a way to test these changes, is there?

include/llvm/Support/MathExtras.h
682

Switch to use doxygen-style comments?

include/llvm/Support/TrailingObjects.h
150

Does this need to be public?

I don't suppose there's a way to test these changes, is there?

It's a utility class (which is not even used yet). I am not aware of testing for the ADTs, etc. aside from using them internally. Perhaps I should mark the change as NFC?

include/llvm/Support/MathExtras.h
682

Okay. I'll need to evaluate the appearance of the FIXME in Doxygen.

include/llvm/Support/TrailingObjects.h
150

This is in an "Impl" class in an "internal" namespace, so I believe leaving it public is reasonable.

I don't suppose there's a way to test these changes, is there?

It's a utility class (which is not even used yet). I am not aware of testing for the ADTs, etc. aside from using them internally. Perhaps I should mark the change as NFC?

We typically test ADTs using unit tests. Check out the ADTTests project in llvm. I definitely would not mark this as NFC -- it has functional changes, they're just not in use yet. If you can devise some tests to add to the ADT unit tests, that would help build confidence that this functionality is ready to be used, and helps protect us against accidental regressions in behavior later. It also exercises these code paths on all the compilers we support, which is probably the most important bit since these changes are a reaction to compiler differences in constexpr support.

include/llvm/Support/TrailingObjects.h
150

Reasonable, sure, but the preference is always to hide implementation details when possible from the public interface.

hubert.reinterpretcast edited edge metadata.

Address review comments: add tests, access control, Doxygen

hubert.reinterpretcast marked an inline comment as done.

Avoid MSVC C4099: replace 'struct' with 'class'

Replace call to LLVM_CONSTEXPR function in constant expression context

Make FixedSizeStorageOwner accessible to test code

Make FixedSizeStorage accessible to test code

FixedSizeStorage and not FixedSizeStorageOwner is what the test code needs. Use alias template to avoid issues with MSVC.

aaron.ballman accepted this revision.Jul 29 2016, 4:33 PM
aaron.ballman edited edge metadata.

LGTM, thank you!

This revision is now accepted and ready to land.Jul 29 2016, 4:33 PM
hubert.reinterpretcast retitled this revision from TrailingObjects::FixedSizeStorage constexpr fixes to TrailingObjects::FixedSizeStorage constexpr fixes + tests.Jul 29 2016, 4:55 PM
hubert.reinterpretcast updated this object.
hubert.reinterpretcast edited edge metadata.