aaron.ballman added a comment. In https://reviews.llvm.org/D22668#500340, @hubert.reinterpretcast wrote:
> In https://reviews.llvm.org/D22668#499164, @aaron.ballman wrote: > > > 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. ================ Comment at: include/llvm/Support/TrailingObjects.h:149 @@ -148,1 +148,3 @@ + struct RequiresRealignment { + static const bool value = ---------------- hubert.reinterpretcast wrote: > aaron.ballman wrote: > > Does this need to be public? > This is in an "Impl" class in an "internal" namespace, so I believe leaving > it public is reasonable. Reasonable, sure, but the preference is always to hide implementation details when possible from the public interface. https://reviews.llvm.org/D22668 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
