This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Fix some triviality computations
AcceptedPublic

Authored by royjacobson on Jul 10 2023, 2:11 PM.

Details

Reviewers
shafik
cor3ntin
Group Reviewers
Restricted Project
Summary

Fix #63352 and one other similar issue by slightly adjusting the computation for the existance of non
trivial special member functions.

Diff Detail

Event Timeline

royjacobson created this revision.Jul 10 2023, 2:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2023, 2:11 PM
royjacobson requested review of this revision.Jul 10 2023, 2:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2023, 2:11 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
royjacobson added a reviewer: Restricted Project.Jul 11 2023, 3:10 AM
cor3ntin accepted this revision.Jul 11 2023, 3:55 AM
cor3ntin added a subscriber: cor3ntin.

This is a bit confusing but I think it is correct.
LGTM modulo typos.

clang/docs/ReleaseNotes.rst
70
This revision is now accepted and ready to land.Jul 11 2023, 3:55 AM
shafik added inline comments.Jul 11 2023, 9:12 AM
clang/include/clang/AST/DeclCXX.h
1269

These references looks like they need to be updated. Same below and it looks like hasNonTrivialCopyConstructorForCall is missing references all together.

royjacobson added inline comments.Jul 23 2023, 12:45 PM
clang/include/clang/AST/DeclCXX.h
1269

TBH I don't think those functions actually need references to the standard? Whether the actual member functions are trivial or not is already calculated before. Do you think I can just remove it? :)

Update release note

philnik added inline comments.
clang/include/clang/AST/DeclCXX.h
1269

I think it makes sense to call out that these functions represent something from the standard. There are other functions with similar names, which don't have an equivalent in the C++ standard, like hasTrivialDestructorForCall.

shafik added inline comments.Jul 24 2023, 2:30 PM
clang/include/clang/AST/DeclCXX.h
1269

The point of having the reference is so folks can understand the logic behind why someone choose to implement the functionality as they did.

I also prefer quoting the critical text since the paragraphs, sometimes the stable name and wording can change and at least having text can allow us to track which standard it came from and hopefully which change changed the text and understand if it effects our compliance or not.

It can be a lot of work to figure this out from scratch if you don't know where to look.

@royjacobson Are you still working on that? It seems it's only missing updates to a comment

I did not have a lot of time for Clang the last few months unfortunately. I might take a look at this again next month when I'll have a bit more time.

I, uh, got preoccupied by some recent events. Don't think I'll have the time for this unfortunately.