This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Clean up ActionResult type a little. NFCI
ClosedPublic

Authored by sammccall on Aug 16 2023, 8:18 AM.

Details

Summary
  • Document the valid states: invalid/unset/pointer (Currently both documentation and implementation strongly suggest that pointer+invalid is poissible, when it's not)
  • Remove unused set() functions, which had different semantics between the compressed/uncompressed specialization! (The former allowing escaping the tristate into pointer+invalid)
  • Make the compressed specialization's internals directly model the tristate, rather than pretending pointer + invalid were independent.
  • Make members of each version identical where possible, remove repetition
  • fix operator= accidentally returning a const reference
  • Fix indentation :-)

This was motivated by D157868, in which an experienced clang dev was
confused about the possible states for ExprResult - and I vividly
remember getting very confused about this myself.

Diff Detail

Event Timeline

sammccall created this revision.Aug 16 2023, 8:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2023, 8:18 AM
sammccall requested review of this revision.Aug 16 2023, 8:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2023, 8:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
kadircet accepted this revision.Aug 16 2023, 9:16 AM

thanks a lot for doing this, i think this makes it a lot more obvious that this is a tristate struct rather than invalid just being a bit that can bit set for both null and non-null pointers.

clang/include/clang/Sema/Ownership.h
155

nit: also initialize Invalid to false?

204

nit: Value == InvalidValue ? nullptr : Value ?

This revision is now accepted and ready to land.Aug 16 2023, 9:16 AM
aaron.ballman accepted this revision.Aug 16 2023, 9:24 AM

Thank you for this! LGTM aside from a tiny nit.

clang/include/clang/Sema/Ownership.h
147

It might be worth tying this to usable.

sammccall marked 2 inline comments as done.Aug 18 2023, 4:21 AM
sammccall added inline comments.
clang/include/clang/Sema/Ownership.h
204

I think that's probably slower, I don't see how the compiler could avoid actually emitting some sort of conditional.

Clang produces cmov: https://godbolt.org/z/croo6o3q6

I'm not 100% sure it matters, but it's a common path we expect to be trivial so i'd rather not change it here.

tbaeder added inline comments.
clang/include/clang/Sema/Ownership.h
180–183
This revision was landed with ongoing or failed builds.Aug 18 2023, 4:31 AM
This revision was automatically updated to reflect the committed changes.