Fix bugzilla issue 24623 followup. #16613

JohanEngelen · 2024-06-22T16:57:05Z

Rename CppRuntime cpp to cxx
Also accept c++ for cxx in target string.

dlang-bot · 2024-06-22T16:57:10Z

Thanks for your pull request and interest in making D better, @JohanEngelen! 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
✓	24623	enhancement	Rename version CppRuntime_Clang/Gcc to CppRuntime_libcxx/libstdcxx.

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#16613"

JohanEngelen · 2024-06-22T17:00:18Z

dlang/dlang.org#3853

Note the annoyance (and errorprone) of not having the spec in same repository as compiler...

Herringway · 2024-06-22T17:04:26Z

isn't this going to cause silent failures in code relying on the old version identifiers? it's too bad a deprecation process isn't possible for them

JohanEngelen · 2024-06-22T17:05:29Z

isn't this going to cause silent failures in code relying on the old version identifiers?

No, because the old ones are still being defined by the compiler.

Herringway · 2024-06-22T17:09:56Z

isn't this going to cause silent failures in code relying on the old version identifiers?

No, because the old ones are still being defined by the compiler.

That doesn't appear to be true with this PR's changes

ibuclaw · 2024-06-22T19:20:23Z

isn't this going to cause silent failures in code relying on the old version identifiers?

No, because the old ones are still being defined by the compiler.

That doesn't appear to be true with this PR's changes

Nothing's been released yet so we're safe until end of the month.

ibuclaw · 2024-06-22T20:00:57Z

compiler/src/dmd/cond.d

-            case "CppRuntime_libcpp":
+            case "CppRuntime_libcxx":
            case "CppRuntime_DigitalMars":
-            case "CppRuntime_libstdcpp":
+            case "CppRuntime_libstdcxx":
            case "CppRuntime_Microsoft":
            case "CppRuntime_Sun":


I've given it a bit more thought.

So just to underline something here, we have CppRuntime_...

DigitalMars

Microsoft

Sun

libcxx

libstdcxx

Whilst I can agree that calling them Clang and Gcc is both vague and misleading (although g++ doesn't work with libc++ AFAIK), I'm not initially convinced they should follow a different style either.

The only alternative I can offer is taking inspiration from their respective documentation pages.

libstdc++ titles itself the GNU C++ Library

libc++ while it does make a reference to other C++ libraries by vendor - Apache libstdcxx, STLport, ... - it only titles itself as the bland C++ Standard Library. However various package distributors seem to almost universally call it the LLVM C++ Standard Library.

So I'm erring on the side of renaming them CppRuntime_LLVM and CppRuntime_GNU. But happy to be convinced otherwise if there's a compelling reason why explicit library names are the best handles to use.

Thanks!

Your proposal is reasonable, I think it's unlike that GNU or LLVM will implement another standard library. Personally, I would not recognize CppRuntime_GNU/LLVM as libstdc++/libc++ as quickly, but it is rare anyway to have to work with this stuff so doesn't really matter.
For -target= recognition, I propose we really do recognize libstdc++/libc++ (and not add gnu/llvm because those are very ambiguous in a triple)

(in the end, I think we do rely on the exact library, not the vendor, that'd be a reason to name the thing by its name)

With the new evidence @ibuclaw provided, I would just err on the side of reverting the initial PR altogether.
I couldn't care less about naming, which is why I was personally okay with your proposal, but will going from CppRuntime_Gcc to CppRuntime_GNU, etc... really make that big of a difference that the change has value ? I don't really think so.

I couldn't care less about naming

Really?? Naming is a big part of programming (a key reason why DMD source is hard to understand and actively avoided by me).

but will going from CppRuntime_Gcc to CppRuntime_GNU, etc... really make that big of a difference that the change has value ?

Yes. The current naming is factually incorrect and hurts legibility of druntime code.

If you don't care about naming, then you also don't have an argument against changing the names, I guess?

Personally, I would not recognize CppRuntime_GNU/LLVM as libstdc++/libc++ as quickly

Though it's not our problem, it's libc++ for the Standard C++ library, then there's libstdc++ for the GNU C++ library - a misleading std. :-)

Whichever way we go, my own preference is to keep the version identifiers consistent if at all possible, so either we have:

CppRuntime_DigitalMars

CppRuntime_Microsoft

CppRuntime_Sun

CppRuntime_LLVM (or Llvm/llvm)

CppRuntime_GNU (or Gnu/gnu)

or it is...

CppRuntime_snn (guessing here)

CppRuntime_msvc

CppRuntime_libCstd

CppRuntime_libcxx

CppRuntime_libstdcxx

Tradeoff table:

| ..._DigitalMars | ..._DigitalMars | ..._snn | ..._DigitalMars_snn | ..._Microsoft | ..._Microsoft | ..._msvc | ..._Microsoft_msvc | ..._Sun | ..._Sun | ..._libCstd | ..._Sun_libCstd | ..._Clang | ..._LLVM | ..._libcxx | ..._LLVM_libcxx | ..._Gcc | ..._GNU | ..._libstdcxx | ..._GNU_libstdcxx --------|-----------------|-----------------|---------------|--------------------- Clarity | 4 | 7 | 6 | 10 # change| 0 | 2 | 5 (all) | 5 (all) backw | n/a | Keep 2 old ones | Keep 5 old? | Keep 5 old? compat? | | | | Remark | incorrect | assume 1 lib | | too verbose? | | per vendor | |

minus 1 for clarity of the lib names, because of less known Sun and DM lib names.

Yes. The current naming is factually incorrect and hurts legibility of druntime code.

But the new names hurts readability for (some) people that develop projects outside of DRuntime, which I would hope is most people! Because when you are linking against C++ source code, you know what compiler you use, and so Clang / Gcc are helpful in that regard. For people only linking, I understand the situation may be different.

If you don't care about naming, then you also don't have an argument against changing the names, I guess?

In this instance, since neither name is completely wrong (IMO, I know you disagree on that point), I care about the amount of change. And I don't think the change from one to the other is materially better. Other people's mileage may vary.

or it is...

CppRuntime_snn (guessing here)

CppRuntime_msvc

CppRuntime_libCstd

CppRuntime_libcxx

CppRuntime_libstdcxx

That, on the other hand, would be pretty bad.

Yes. The current naming is factually incorrect and hurts legibility of druntime code.

But the new names hurts readability for (some) people that develop projects outside of DRuntime, which I would hope is most people! Because when you are linking against C++ source code, you know what compiler you use, and so Clang / Gcc are helpful in that regard.

This is exactly what is wrong about it!
Let's say you are using Clang, what CppRuntime_* do you most likely need do you think? Hint: it is CppRuntime_Gcc.

Personally, I would not recognize CppRuntime_GNU/LLVM as libstdc++/libc++ as quickly, but it is rare anyway to have to work with this stuff so doesn't really matter.

WRT one library per vendor, more likely there would be different versions - for example GNU's library is specifically libstdc++v3; v2 was pre-GCC 3.x.

For -target= recognition, I propose we really do recognize libstdc++/libc++ (and not add gnu/llvm because those are very ambiguous in a triple)

Agreed.

Rename CppRuntime to _LLVM and _GNU Also accept c++ for cxx in target string.

JohanEngelen · 2024-06-28T16:45:22Z

I've changed the naming to
CppRuntime_DigitalMars
CppRuntime_Microsoft
CppRuntime_Sun
CppRuntime_LLVM
CppRuntime_GNU

ibuclaw · 2024-06-30T11:45:04Z

Let's go with this, as it's an improvement over the current.

Version identifier cpp -> LLVM/GNU, see dlang/dmd#16613 Signed-off-by: Iain Buclaw <ibuclaw@users.noreply.github.com> Merged-on-behalf-of: Iain Buclaw <ibuclaw@users.noreply.github.com>

JohanEngelen requested a review from ibuclaw as a code owner June 22, 2024 16:57

dlang-bot added the Severity:Enhancement label Jun 22, 2024

JohanEngelen added a commit to JohanEngelen/dlang.org that referenced this pull request Jun 22, 2024

Version identifier cpp -> cxx, see dlang/dmd#16613

2313ee3

JohanEngelen mentioned this pull request Jun 22, 2024

Rename CppRuntime_Clang/Gcc to CppRuntime_libcpp/libstdcpp. #16610

Merged

ibuclaw reviewed Jun 22, 2024

View reviewed changes

Fix bugzilla issue 24623 followup.

988f31a

Rename CppRuntime to _LLVM and _GNU Also accept c++ for cxx in target string.

JohanEngelen force-pushed the cppruntime_rename2 branch from ea3e2ac to 988f31a Compare June 28, 2024 16:44

ibuclaw approved these changes Jun 30, 2024

View reviewed changes

ibuclaw added the Merge:auto-merge label Jun 30, 2024

dlang-bot merged commit b5130fd into dlang:master Jun 30, 2024
41 checks passed

JohanEngelen deleted the cppruntime_rename2 branch June 30, 2024 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bugzilla issue 24623 followup. #16613

Fix bugzilla issue 24623 followup. #16613

JohanEngelen commented Jun 22, 2024

dlang-bot commented Jun 22, 2024 •

edited

Loading

JohanEngelen commented Jun 22, 2024

Herringway commented Jun 22, 2024

JohanEngelen commented Jun 22, 2024

Herringway commented Jun 22, 2024

ibuclaw commented Jun 22, 2024

ibuclaw Jun 22, 2024 •

edited

Loading

JohanEngelen Jun 23, 2024

JohanEngelen Jun 23, 2024

Geod24 Jun 23, 2024

JohanEngelen Jun 23, 2024

ibuclaw Jun 23, 2024 •

edited

Loading

JohanEngelen Jun 23, 2024

Geod24 Jun 24, 2024

JohanEngelen Jun 24, 2024

ibuclaw Jun 27, 2024

JohanEngelen commented Jun 28, 2024

ibuclaw commented Jun 30, 2024

Fix bugzilla issue 24623 followup. #16613

Fix bugzilla issue 24623 followup. #16613

Conversation

JohanEngelen commented Jun 22, 2024

dlang-bot commented Jun 22, 2024 • edited Loading

Bugzilla references

Testing this PR locally

JohanEngelen commented Jun 22, 2024

Herringway commented Jun 22, 2024

JohanEngelen commented Jun 22, 2024

Herringway commented Jun 22, 2024

ibuclaw commented Jun 22, 2024

ibuclaw Jun 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibuclaw Jun 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanEngelen commented Jun 28, 2024

ibuclaw commented Jun 30, 2024

dlang-bot commented Jun 22, 2024 •

edited

Loading

ibuclaw Jun 22, 2024 •

edited

Loading

ibuclaw Jun 23, 2024 •

edited

Loading