This is an archive of the discontinued LLVM Phabricator instance.

Avoid redundant reference to isPodLike in SmallVect implementation
ClosedPublic

Authored by serge-sans-paille on Nov 28 2018, 7:14 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

include/llvm/ADT/SmallVector.h
188 ↗(On Diff #175688)

I would have left this more similar to the left-hand side: i.e., replace lines 183-188 with simply

template <typename T, bool = isPodLike<T>::value>
class SmallVectorTemplateBase : public SmallVectorTemplateCommon<T> {

What you've got is equivalent, of course; but my suggestion was intended to reduce repetition, so it seems silly to add a repetition here.

242 ↗(On Diff #175688)

I would not have changed these two lines. But you could argue that the new version is more self-documenting, so okay.

My comment on D54472 didn't mention it because I thought it would be obvious, but the same pattern applies to your proposed diffs in "include/llvm/ADT/Optional.h".

serge-sans-paille marked an inline comment as done.Nov 29 2018, 5:19 AM
serge-sans-paille added inline comments.
include/llvm/ADT/SmallVector.h
188 ↗(On Diff #175688)

I tend to prefer explicit over implicit, thus the two specialization and forward declaration. I'd actually even prefer a std::conditional and two base classes with different names.

Anyway, I applied the modification you suggested, no need to debate too much about such details :-)

Quuxplusone accepted this revision.Nov 29 2018, 9:15 AM

LGTM, for what that's worth!

This revision is now accepted and ready to land.Nov 29 2018, 9:15 AM
This revision was automatically updated to reflect the committed changes.