[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

2022-07-20 Thread Tom Honermann via Phabricator via cfe-commits
tahonermann added a comment. > Personally, the problem is not serializing class template specializations > "too early". The problem is that we are deserializing class template > specializations "too early". Yes, I agree. > The key point here is that Modules would read declarations lazily for

[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

2022-07-19 Thread Chuanqi Xu via Phabricator via cfe-commits
ChuanqiXu added a comment. In D129748#3662498 , @erichkeane wrote: > I'll note that I don't "like" the idea, so much as I see this patch as an > 'improvement' to unblock efforts with additional value, though not much of > one. I suspect as Aaron does

[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

2022-07-19 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added a comment. I'll note that I don't "like" the idea, so much as I see this patch as an 'improvement' to unblock efforts with additional value, though not much of one. I suspect as Aaron does that there is a significant problem that needs fixing here with how our serialization wo

[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

2022-07-19 Thread Chuanqi Xu via Phabricator via cfe-commits
ChuanqiXu added a comment. In D129748#3660684 , @aaron.ballman wrote: > The argument type is `TypeArgument`, not `TypdefType`. The guts of the > reading code for the preferred name attribute *should* be: Oh, My bad. I didn't mention it in the above res

[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

2022-07-18 Thread Chuanqi Xu via Phabricator via cfe-commits
ChuanqiXu added a comment. In D129748#3660650 , @tahonermann wrote: > I neglected to explicitly mention in conjunction with my last comment, but > @ChuanqiXu, can you check to see if we are indeed serializing class template > specializations "too early

[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

2022-07-18 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In D129748#3658657 , @ChuanqiXu wrote: > In D129748#3654918 , @aaron.ballman > wrote: > >> For example, I would be IBOutletCollection, OwnerAttr, PointerAttr, and >> TypeTagForData

[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

2022-07-18 Thread Tom Honermann via Phabricator via cfe-commits
tahonermann added a comment. I neglected to explicitly mention in conjunction with my last comment, but @ChuanqiXu, can you check to see if we are indeed serializing class template specializations "too early" and, if so, whether delaying such serialization until a defining point resolves the is

[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

2022-07-18 Thread Tom Honermann via Phabricator via cfe-commits
tahonermann added a comment. Thank you for the detailed explanation, @ChuanqiXu, that was very helpful. It looks to me like the problem may be that the initial declaration of the `basic_string_view` class template is non-defining, but when serializing that declaration, we serialize a definition

[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

2022-07-17 Thread Chuanqi Xu via Phabricator via cfe-commits
ChuanqiXu added a comment. In D129748#3654918 , @aaron.ballman wrote: > For example, I would be IBOutletCollection, OwnerAttr, PointerAttr, and > TypeTagForDatatypeAttr all behave the same way as they all take a type > argument. I don't think we want t

[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

2022-07-15 Thread Tom Honermann via Phabricator via cfe-commits
tahonermann added a comment. I'm not following what the root problem is. You stated: > When we write the attribute preferred_name(foo) in ASTWriter, the compiler > would try to write the type for the argument foo. Then when the compiler > write the type for foo, the compiler find the type for f

[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

2022-07-15 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In D129748#3654909 , @erichkeane wrote: > I think there _IS_ perhaps an acceptability to ignoring this attribute when > it would cause a problem. However, I think doing it as you're doing it now > is wrong. IF we are go

[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

2022-07-15 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added a comment. In D129748#3653897 , @ChuanqiXu wrote: > In D129748#3651771 , @erichkeane > wrote: > >> I guess I don't have a good idea why this attribute would cause ODR issues? >> It would seem t

[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

2022-07-14 Thread Chuanqi Xu via Phabricator via cfe-commits
ChuanqiXu added a comment. In D129748#3651771 , @erichkeane wrote: > I guess I don't have a good idea why this attribute would cause ODR issues? > It would seem that if it appeared in 2 different TUs that we could just > 'pick' whichever we wanted, ri

[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

2022-07-14 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added a comment. I guess I don't have a good idea why this attribute would cause ODR issues? It would seem that if it appeared in 2 different TUs that we could just 'pick' whichever we wanted, right? The description in the bug report of the problem isn't clear to me what the actual

[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

2022-07-14 Thread Chuanqi Xu via Phabricator via cfe-commits
ChuanqiXu added a comment. Or is it better to add a special option `-fno-preferred-name` to ignore it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129748/new/ https://reviews.llvm.org/D129748 ___ cfe-c

[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

2022-07-14 Thread Chuanqi Xu via Phabricator via cfe-commits
ChuanqiXu created this revision. ChuanqiXu added reviewers: rsmith, vsapsai, ilya-biryukov, erichkeane, iains, v.g.vassilev. ChuanqiXu added a project: clang-modules. Herald added a reviewer: aaron.ballman. Herald added a project: All. ChuanqiXu requested review of this revision. Herald added a pr