This is an archive of the discontinued LLVM Phabricator instance.

[C++20][Modules] Do not allow non-inline external definitions in header units.
ClosedPublic

Authored by iains on Dec 17 2022, 8:50 AM.

Details

Summary

[module.import/6] last sentence:
A header unit shall not contain a definition of a non-inline function or
variable whose name has external linkage.

Diff Detail

Event Timeline

iains created this revision.Dec 17 2022, 8:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2022, 8:50 AM
iains published this revision for review.Dec 17 2022, 8:52 AM
iains added a reviewer: ChuanqiXu.
iains added a subscriber: Restricted Project.

this came up during discussion of other header unit constraints, we had not implemented it yet.
found a test case where we'd accidentally broken this rule too.

Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2022, 8:54 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ChuanqiXu accepted this revision.Dec 18 2022, 7:08 PM

LGTM generally.

clang/lib/Sema/SemaDecl.cpp
12957–12958

getLangOpts().CPlusPlus20 is redundant. It is also good to define a helper interface withinHeaderUnit in Sema (not required).

15116–15117

ditto

This revision is now accepted and ready to land.Dec 18 2022, 7:08 PM
iains added inline comments.Dec 19 2022, 1:07 AM
clang/lib/Sema/SemaDecl.cpp
12957–12958

I do not mind making this change - but note that the constraint is specific to C++20 and there are some people who want to remove it (the constraint). I guess you meant getCurrentModule() ?

ChuanqiXu added inline comments.Dec 19 2022, 1:14 AM
clang/lib/Sema/SemaDecl.cpp
12957–12958

I do not mind making this change - but note that the constraint is specific to C++20 and there are some people who want to remove it (the constraint).

Got it. Let's make it when it comes true. It won't be a big deal.

I guess you meant getCurrentModule() ?

It looks a little bit better to me to not access ModuleScopes directly.

iains updated this revision to Diff 483909.Dec 19 2022, 4:29 AM

rebased, addressed comments.

iains marked 2 inline comments as done.Dec 19 2022, 4:31 AM

OK so this is what I plan to land assuming testing goes OK.
I suspect that this might cause some user code to flag errors - there are quite a number of ODR violations "in the wild".

clang/lib/Sema/SemaDecl.cpp
12957–12958

revised now (I added the helper).

OK so this is what I plan to land assuming testing goes OK.
I suspect that this might cause some user code to flag errors - there are quite a number of ODR violations "in the wild".

I forgot we need to mention such changes in https://clang.llvm.org/docs/ReleaseNotes.html#potentially-breaking-changes.

iains marked an inline comment as done.Dec 19 2022, 10:23 PM

OK so this is what I plan to land assuming testing goes OK.
I suspect that this might cause some user code to flag errors - there are quite a number of ODR violations "in the wild".

I forgot we need to mention such changes in https://clang.llvm.org/docs/ReleaseNotes.html#potentially-breaking-changes.

It will not break any existing compilation (we accept headers with non-inline external definitions as textual includes, that has not changed). What I mean is that such headers will not be accepted as Header Units - and I am sure some users will consider that to be a bug (even though the problem is that the header is not ODR safe).
So it's not a change that breaks existing code - because no-one has Header Units in existing code.

So it's not a change that breaks existing code - because no-one has Header Units in existing code.

We can't assume this. Since we already said people can try to use header units in clang15. We can't assume there is nobody using header units just because they don't tell us. In fact, I've seen some bug reports about header units recently. So there are people (try to) using header units at least. So I think we should better to mark it in the potential breaking section.

iains updated this revision to Diff 486202.Jan 4 2023, 2:02 AM

rebase, added release notes

iains added a comment.Jan 4 2023, 2:03 AM

how does that look?

ChuanqiXu accepted this revision.Jan 4 2023, 2:24 AM

Yeah, it looks good to me.