This is an archive of the discontinued LLVM Phabricator instance.

[C++20] [Modules] Don't complain about duplicated default template argument across modules
ClosedPublic

Authored by ChuanqiXu on Jan 24 2022, 4:12 AM.

Details

Summary

See https://github.com/cplusplus/draft/pull/5204 for a detailed background.

Simply, the test redundant-template-default-arg.cpp attached to this patch should be accepted instead of being complained about the redefinition.

Diff Detail

Event Timeline

ChuanqiXu requested review of this revision.Jan 24 2022, 4:12 AM
ChuanqiXu created this revision.
urnathan added inline comments.Jan 25 2022, 5:41 AM
clang/lib/Sema/SemaTemplate.cpp
2741–2757

while I do approve of good commenting, this is a little wordy and I think misleading. It is only from global module fragments that you can get duplicate declarations and those must match default parameters (I think). so perhaps 'IsGlobalModule'? or somehting like that.

Perhaps all of this comment shoould be at the point of checking? After all the preceding comment mentions 'variables used to ...'?

2932

Yes, I think this is a better place for the comment. If there are duplicates we need to check they are the same -- pedantically the same tokens, but at least AST equivalence?

ChuanqiXu planned changes to this revision.Jan 26 2022, 1:23 AM
ChuanqiXu added inline comments.
clang/lib/Sema/SemaTemplate.cpp
2932

Yeah, it should be necessary to check the ODR. I found it is not trivial to do so. It needs more work. I sent https://reviews.llvm.org/D118223 as a forward patch.

Address comment to check if the duplicated default template arguments are the same and add tests to address the negative cases.

ChuanqiXu edited the summary of this revision. (Show Details)Feb 15 2022, 11:04 PM
ChuanqiXu added a reviewer: iains.
urnathan added inline comments.Feb 16 2022, 6:54 AM
clang/lib/Sema/SemaTemplate.cpp
2673–2675

Can this ever trigger and the below expression test not trigger, given the above type comparison?

2677–2701

I'm kind of surprised how complex this check is. Isn't there an AST comparator available somewhere?

Use ODRHash to simplify code.

clang/lib/Sema/SemaTemplate.cpp
2677–2701

I found ODRHash. I think it is much better now.

urnathan added inline comments.Feb 17 2022, 4:05 AM
clang/lib/Sema/SemaTemplate.cpp
2677–2701

hm that suggests there there must be a comparator too -- this isn't a cryptographically strong hash is it? How would the compiler currently make use of 'definitely different' and 'probably the same' without such a comparator?

ChuanqiXu added inline comments.Feb 17 2022, 4:13 AM
clang/lib/Sema/SemaTemplate.cpp
2677–2701

Yeah, I am sure there is not an such comparator. Or it has some methods like: ASTContext::hasSameType for type and ASTContext::isSameEntity() for Decl. But it lacks such methods now for Stmt and Expr.

How would the compiler currently make use of 'definitely different' and 'probably the same' without such a comparator?

Now it uses the two methods I listed above and ODRHash to compare. I think the two methods works for 'definitely different' and ODRHash works for 'probably the same'. So it's the reason why my previous implementation looks lengthy. Since I want to handle it by hand. (The previous method only works for simple Expr. I think it would be large work to implement comparator for whole Expr or Stmt).

urnathan added inline comments.Feb 17 2022, 6:01 AM
clang/lib/Sema/SemaTemplate.cpp
2677–2701

Hm, how do template instantations work -- there must be some way of determining 'is this instantation just here the same as one I've already seen'

urnathan added inline comments.Feb 17 2022, 6:58 AM
clang/lib/Sema/SemaTemplate.cpp
2677–2701

also, the same functionality (with more generality) is needed for comparing regular function default arguments with multiple definitions in the GM. How is that done (or is it yet to be implemented?)

ChuanqiXu added inline comments.Feb 18 2022, 1:21 AM
clang/lib/Sema/SemaTemplate.cpp
2677–2701

also, the same functionality (with more generality) is needed for comparing regular function default arguments with multiple definitions in the GM. How is that done (or is it yet to be implemented?)

We implemented ODR check in ASTReader by ODRHash.

Hm, how do template instantations work -- there must be some way of determining 'is this instantation just here the same as one I've already seen'

First, previously we don't allow the redefinition of template default argument. And for the case without template default argument, previously we would try find the definition for the synthesized type, if we could find one, use it. And if we couldn't find one, initialize one.


Given we already checks ODR by ODRHash in ASTReader, I think what we do here might not be bad. I agree that it is odd at the first sight to use Hash to detect difference from the perspective of CS. But if this is what we used to do, I think it is acceptable.

urnathan added inline comments.Feb 18 2022, 6:14 AM
clang/lib/Sema/SemaTemplate.cpp
2677–2701

indeed. I would be more comfortable with a comment by someone more familiar with this @rsmith ?

ChuanqiXu marked an inline comment as done.Feb 20 2022, 10:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 6:19 PM

@urnathan It looks @rsmith is not available this time. Would you like to accept this one?

urnathan resigned from this revision.Mar 9 2022, 10:53 AM
ChuanqiXu added a reviewer: Restricted Project.Mar 20 2022, 11:15 PM
rsmith added inline comments.Jun 2 2022, 12:25 PM
clang/lib/Sema/SemaTemplate.cpp
2669

ODR hashing is intended to produce a hash that's stable across compilations (so it can't use pointer comparisons to check types are identical, for example). For use cases that don't need a hash code that's stable across processes, using Stmt::Profile should be preferred. Can you use that here?

2738–2747

Please rephrase these comments to remove the "Variables used to" part and to explain instead what value is stored in the variable. (Eg: "Whether we've seen a duplicate default argument in the same translation unit.")

2779–2787

This is the wrong thing to check -- whether the owning module is imported (from an AST file) is irrelevant. We might parse multiple translation units in the same compilation, and we might split a single translation unit into multiple compilations (eg with a precompiled preamble) resulting in some of the same translation unit being in an AST file. What matters is whether the declaration is in the current translation unit or a different translation unit.

I'm not sure what the best way to detect that is currently. You might be able to just compare getOwningModule() of the two declarations, but I don't think that will properly handle cases in the global module fragment, or cases where one template is in the primary module interface unit and another is in a module implementation unit of the same module.

Ideally I would like for us to create one TranslationUnitDecl per translation unit, rather than shoving all imported code into the translation unit of the importer, but that's a moderately large change, which I'd like to not block this change on if we can avoid it. But I think that's the right longer-term approach. Then here we would just need to find the lexically-enclosing TranslationUnitDecl for each template parameter and see if they're the same.

ChuanqiXu updated this revision to Diff 434411.Jun 6 2022, 2:59 AM

Address comments.

ChuanqiXu marked 10 inline comments as done.Jun 6 2022, 3:06 AM
ChuanqiXu added inline comments.
clang/lib/Sema/SemaTemplate.cpp
2779–2787

I found we could determine whether the declaration is in the current TU or not by using Sema::ModuleScopes, which contains the modules we're currently parsing.

ChuanqiXu updated this revision to Diff 434415.Jun 6 2022, 3:16 AM
ChuanqiXu marked an inline comment as done.

Remove unused header ODRHash.h

Given @rsmith and @iains is busy and this revision is relatively small, innocent and looks good to me, I plan to land this in Friday in case there is no further comments.

ChuanqiXu updated this revision to Diff 443123.Jul 7 2022, 8:09 PM

Minor changes.

ChuanqiXu updated this revision to Diff 443124.Jul 7 2022, 8:11 PM

Minor changes.

ChuanqiXu accepted this revision.Jul 7 2022, 8:12 PM
This revision is now accepted and ready to land.Jul 7 2022, 8:12 PM