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

[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 performance reasons. This is a feature of modules.

Indeed it is, and an important one. I don't think laziness poses a problem though; the issue is ensuring that dependencies are (de)serialized in a proper order. In this case, that implies that, at least for a class template declared with the `__preferred_name__` attribute, a non-defining declaration must be deserialized before the defining declaration.

I can imagine several ways in which things may be going wrong. Perhaps (de)serialization is switching to a canonical declaration when it should not be (e.g., reading/writing a definition when only a declaration should be). Perhaps, for implicitly instantiated declarations (at least for ones for which `__preferred_name__` is attached), separate non-defining and defining declarations should be (de)serialized (in the dumps above, there appears to only be a single address associated with the `basic_string_view<char>` specialization). Perhaps `ClassTemplateSpecializationDecl` should be (de)serialized such that a (valid) non-defining declaration is created before attempting to load anything else that might lead to recursion to the same declaration (the intent being that an early return when recursion is detected still yields a valid incomplete declaration).

I briefly looked at some code, but (unsurprisingly) did not see anything obvious. I am inclined towards finding a better fix for this before accepting it. If a good fix isn't identified for the initial Clang 15 release, I think an argument can be made for including it in a Clang 15.0.X release.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129748/new/

https://reviews.llvm.org/D129748