Page MenuHomePhabricator

[Sema] Simplify CheckConstraintSatisfaction. NFC
ClosedPublic

Authored by ilya-biryukov on May 4 2022, 6:58 AM.

Details

Summary
  • Exit early when constraint caching is disabled.
  • Use unique_ptr to manage temporary lifetime.
  • Fix a typo in a comment (InsertPos instead of InsertNode).

The new code duplicates the forwarding call to CheckConstraintSatisfaction,
but reduces the number of interconnected if statements and simplifies lifetime
management.

This increases the overall readability.

Diff Detail

Event Timeline

ilya-biryukov created this revision.May 4 2022, 6:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2022, 6:58 AM
ilya-biryukov requested review of this revision.May 4 2022, 6:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2022, 6:58 AM
ilya-biryukov added inline comments.May 4 2022, 7:01 AM
clang/lib/Sema/SemaConcept.cpp
335

I also wonder if this could be allocated in the AST arena, maybe worth exploring.
Definitely outside this change, though, would like to keep this one an NFC.

sammccall accepted this revision.May 4 2022, 8:06 AM

Thanks for cleaning up the scary new/deletes.

clang/lib/Sema/SemaConcept.cpp
321

Another loose end that could be cleaned up sometime!

ConceptSatisfactionCaching is on by default and controlled by an -f flag.
It was added in https://reviews.llvm.org/D72552 - essential for performance, unclear whether the caching scheme was allowed.

Probably this option should be removed (and if needed, the caching scheme semantics made to match what was standardized)

335

not just could, but must - FoldingSet doesn't own its objects, so this object is leaked once the FoldingSet is destroyed! Allocating it on the ASTContext would fix this because they have the same lifetime.

Agree with not mixing it into this patch, but please add a FIXME and do follow up on it if you don't mind.
(and at that point, should we really be passing it back by value?)

This revision is now accepted and ready to land.May 4 2022, 8:06 AM
  • add FIXME for a memory leak
clang/lib/Sema/SemaConcept.cpp
321

Good point, thanks. I'll ask around to see whether the committee discussions led anywhere.

335

Oh, wow, good catch, thanks. I'll send a separate fix for the memory leak.

This revision was landed with ongoing or failed builds.May 4 2022, 8:53 AM
This revision was automatically updated to reflect the committed changes.
ilya-biryukov added inline comments.May 4 2022, 10:22 AM
clang/lib/Sema/SemaConcept.cpp
335

So there's no memory leak actually.
Sema's destructor calls delete for all nodes inside SatisfactionCache. Allocating them in ASTContext is not trivial as they store a SmallVector, which may dynamically allocate itself. Hence, we must call destructor and ASTContext does not do that.
The SmallVector itself gets built recursively when calling CheckConstraintSatisfaction.

Cleaning it up is probably possible, but a bit more work than I planned for this. I'll replace the fixme with a comment mentioning Sema destructor cleans up for us.

In the future, when doing Concepts stuff, do you mind adding me as a reviewer? This ended up being a somewhat surprising/painful merge conflict with the deferred concepts implementation that I've been trying to get in (D119544).