This is an archive of the discontinued LLVM Phabricator instance.

AST: Avoid using SmallVector::set_size() in UnresolvedSet
ClosedPublic

Authored by dexonsmith on Dec 8 2021, 1:02 PM.

Details

Summary

Update UnresolvedSet to use (and expose) SmallVector::truncate() instead
of SmallVector::set_size(). The latter is going to made private in a
future commit to avoid misuse.

Currently depends on https://reviews.llvm.org/D115383. It seems to improve readability to use truncate() instead of resize() (known to shrink the size), and it keeps an assertion in place (not sure of value?) that this doesn't grow. I can't imagine the former's performance characteristics are important here, so I could be convinced to just use resize()...

Blocker for https://reviews.llvm.org/D115380.

Diff Detail

Event Timeline

dexonsmith requested review of this revision.Dec 8 2021, 1:02 PM
dexonsmith created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2021, 1:02 PM
dexonsmith updated this revision to Diff 392977.Dec 8 2021, 4:38 PM

Adding a couple of other possible reviewers.

[no change, just retriggering bots after spurious failure]

dexonsmith added inline comments.Dec 8 2021, 4:45 PM
clang/lib/Sema/SemaLookup.cpp
623

Two things to confirm here.

First is that the destructors are trivial. From clang/include/clang/AST/DeclAccessPair.h:

class DeclAccessPair {
  uintptr_t Ptr; // we'd use llvm::PointerUnion, but it isn't trivial

(If they hadn't been trivial, then hypothetically there could been other code somewhere that ran the destructors later...)

Second is that set_size() was only used for truncation. I confirmed that that three ways:

  • Looking backward, N starts as Decls.size() and the only changes are decrement operatoers.
  • Looking forward, there's no code that would initialize / assign to the new member (so if it increased size, it would likely have led to problems elsewhere).
  • Tests pass.
dblaikie accepted this revision.Dec 9 2021, 11:29 AM

Looks good.

clang/lib/Sema/SemaLookup.cpp
623

Thanks for the details!

This revision is now accepted and ready to land.Dec 9 2021, 11:29 AM