Page MenuHomePhabricator

[WIP][C++] Implement "Deducing this" (P0847R7)
Needs ReviewPublic

Authored by cor3ntin on Jan 1 2023, 3:22 PM.

Details

Reviewers
shafik
NoQ
aaron.ballman
Group Reviewers
Restricted Project
Summary

This patch implements P0847R7 (partially),
CWG2561 and CWG2653.

The patch is mostly feature complete
(except for explicit parameters in coroutines and mangling),
but obviously it's lacking in tests.

I'm hoping for early fedback and hopefully
finding an agreement on mangling.

Diff Detail

Event Timeline

cor3ntin created this revision.Jan 1 2023, 3:22 PM
Herald added a reviewer: NoQ. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
cor3ntin requested review of this revision.Jan 1 2023, 3:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 1 2023, 3:22 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
cor3ntin added reviewers: aaron.ballman, Restricted Project.Jan 1 2023, 3:24 PM
cor3ntin updated this revision to Diff 485871.Jan 2 2023, 9:28 AM

Fix variable left uninitialized when loading a module

cor3ntin updated this revision to Diff 485951.Jan 3 2023, 3:38 AM

Implement CWG2553/ CWG2554

cor3ntin updated this revision to Diff 485981.Jan 3 2023, 6:29 AM

Implement MSVC mangling scheme for explicit object parameters

I did a look through, I'm quite sure I'm not going to be able to accept this, but I'm hoping the little feedback I gave helped. I can't help with the review of the mangling unfortunately, we probably need to wait for the owners of the manglings to make a decision.

clang/include/clang/AST/Decl.h
1815

What cases are there where we wouldn't have a source location here? Is that case common enough that this should be a default parameter?

clang/include/clang/AST/Expr.h
1447

This bit concerns me a touch, I'm a little scared/shocked that this would matter at all... This makes me think more nefarious issues are at play here.

clang/include/clang/Sema/Sema.h
2130

These params are definitely going to need documentation, I have no idea what that means.

clang/lib/AST/MicrosoftMangle.cpp
2560

This bit probably needs to wait until microsoft decides what the mangling for these are (all of microsoft mangle)

clang/lib/CodeGen/CGExpr.cpp
2633

Eh?

4254

If there is a CodeGen expert in the house, I'd really hope they go through this :)

cor3ntin added inline comments.Jan 3 2023, 7:23 AM
clang/include/clang/AST/Expr.h
1447

See https://eel.is/c++draft/temp.dep.expr#3.6
I think this is because we don't know if the capture is mutable until the type of the object parameter is known.
I still need to find a way to devise a test for that though.

The way we do that is we save that bit and then in computeDependence we apply that rule.
(This bit doesn't need to be serialized once the dependence is computed)

clang/lib/AST/MicrosoftMangle.cpp
2560

I've been chatting with Cameron, microsoft has decided :)
I still have to hear back from the GCC/Itanium folks though

clang/lib/CodeGen/CGExpr.cpp
2633

I need to remove this commented code.
the removed code was just moved around. we had 2 functions doing the same thing, now this one just forward to the non-static one.
I should get rid of it entirely actually.

4254

Oh, me too!
I need to properly write tests for it too. And you know how terrible I am at code gen tests...

erichkeane added inline comments.Jan 3 2023, 7:25 AM
clang/lib/AST/MicrosoftMangle.cpp
2560

Ah! Good! See whatever you can get to make sure we match though!

As far as Itanium, did you try posting on the Itanium github?

cor3ntin added inline comments.Jan 3 2023, 7:32 AM
clang/lib/AST/MicrosoftMangle.cpp
2560

I wrote the tests from the microsoft implementation https://compiler-explorer.com/z/4df8f3e1j
The github issue is there https://github.com/itanium-cxx-abi/cxx-abi/issues/148 - I also mailed a few folks this morning

pbl-pw added a subscriber: pbl-pw.Jan 5 2023, 4:37 PM
rymiel added a subscriber: rymiel.Feb 22 2023, 11:24 PM