dmd icon indicating copy to clipboard operation
dmd copied to clipboard

Fix issue 22550 - tail const C++ class not usable on Windows

Open tim-dlang opened this issue 3 years ago • 7 comments

This changes the C++ mangling of const(Class) from "Class const * const" to "Class const *" for Windows, but only if the class type is used at the top of a type. It is a breaking change for code using "Class const * const" in C++, but this code seems to be less common.

tim-dlang avatar Nov 28 '21 11:11 tim-dlang

Thanks for your pull request and interest in making D better, @tim-dlang! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
22550 normal tail const C++ class not usable on Windows

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#13369"

dlang-bot avatar Nov 28 '21 11:11 dlang-bot

See also issue 21200 and the discussion at https://digitalmars.com/d/archives/digitalmars/D/const_Class_is_mangled_as_Class_const_const_299139.html.

tim-dlang avatar Nov 28 '21 11:11 tim-dlang

Unfortunately this could also affect the D compilers, because DsymbolTable.lookup and DsymbolTable.insert use "Identifier const * const". The change could make bootstrapping harder.

Header generation would also need to be changed.

tim-dlang avatar Nov 28 '21 12:11 tim-dlang

What's the behaviour on itanium? Windows should just mangle it in the same way instead of doing something different.

ibuclaw avatar Nov 28 '21 12:11 ibuclaw

What's the behaviour on itanium? Windows should just mangle it in the same way instead of doing something different.

The outer const is already removed for itanium. See https://github.com/dlang/dmd/blob/master/src/dmd/cppmangle.d#L1464-L1465

But there seems to be a difference between GCC on Linux and Visual C++ on Windows: GCC mangles "void test(const C *c):" and "void test(const C *const c)" the same. Visual C++ uses a different mangling for them.

tim-dlang avatar Nov 28 '21 12:11 tim-dlang

This seems like a major language change here. cc @WalterBright

12345swordy avatar Nov 28 '21 17:11 12345swordy

While implementing header generation for the changed types, I realised that this change would break more code, than I initially thought. Previously I only thought about bindings to existing C++ libraries, but extern(C++) can also be used for D libraries, which are used from C++ (like the DMD frontend). Those C++ headers for D libraries already use declarations with full const and would break with this change.

To prevent breaking existing code, I'm now thinking about making the change optional. This could be done with a compiler recognized UDA, but that would also be limited to class types used directly as return or parameter types. A more general solution would be an extension of the type system. A new trait could be used to create tail const class types, like __trait(headMutable, const(C)). Allowing tail const/immutable/shared for classes has also been proposed in issue 5325 and could replace most uses of std.typecons.Rebindable. The semantics of tail const class types should match tail const pointers to structs. Would such a change require a DIP?

tim-dlang avatar Dec 11 '21 13:12 tim-dlang