Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add -std-c++= #9130

Merged
merged 1 commit into from
Dec 28, 2018
Merged

add -std-c++= #9130

merged 1 commit into from
Dec 28, 2018

Conversation

thewilsonator
Copy link
Contributor

@thewilsonator thewilsonator commented Dec 24, 2018

cc @TurkeyMan @CyberShadow @jacob-carlborg @Geod24 @ibuclaw @WalterBright

Alternative to #9029

(I haven't actually tested it because I can't build dmd (due to a lack of disk space I think, I'm working on it)) Hmm, nope it jut keep OOMing. No it doesn't like -s in Options.

tested, it works.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @thewilsonator! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + dmd#9130"

changelog/stdcpp.dd Outdated Show resolved Hide resolved
changelog/stdcpp.dd Outdated Show resolved Hide resolved
@WalterBright
Copy link
Member

I don't see the advantage of -stdc++=c++98 over -std=c++98
Nobody is going to be confused that the latter does somehow not mean C++. There's no need to say c++ twice.

@thewilsonator
Copy link
Contributor Author

See the other thread.

Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall well done, with the design I expected, just a few suggestions to make it even better:

src/dmd/globals.d Show resolved Hide resolved
src/dmd/globals.h Outdated Show resolved Hide resolved
src/dmd/globals.h Outdated Show resolved Hide resolved
src/dmd/target.d Outdated Show resolved Hide resolved
@thewilsonator
Copy link
Contributor Author

@ZombineDev Done.

Copy link
Contributor

@jacob-carlborg jacob-carlborg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no tests.

src/dmd/globals.h Outdated Show resolved Hide resolved
@thewilsonator
Copy link
Contributor Author

@jacob-carlborg test added.

@thewilsonator
Copy link
Contributor Author

Any last comments?

@ibuclaw
Copy link
Member

ibuclaw commented Dec 28, 2018

Only that there's only one person to convince in signing this off.

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm following this discussion for a while now and the amount of time spent on discussing the flag name is shocking. This behavior is what drove many contributors away in the past.

@thewilsonator
Copy link
Contributor Author

CI failure is unrelated.

@thewilsonator thewilsonator merged commit d7bf157 into dlang:master Dec 28, 2018
@WalterBright
Copy link
Member

See the other thread.

The other thread does not mention a -std++ flag.

It's redundant and doesn't look good. It's not what C++ compilers use. I suggest reversion.

@thewilsonator
Copy link
Contributor Author

This is -std-c++ not -std++, also try to look a bit harder there were lots of things proposed that were similar:
#9029 (comment)
#9029 (comment)
#9029 (comment)
#9029 (comment)
#9029 (comment)
#9029 (comment)
#9029 (comment)

@WalterBright
Copy link
Member

I'm sorry, I don't see -std-c++= either in that thread. Nor is it a C++ compiler switch. Nor have I seen any exposition for -std-c++=c++98. It turns out that this proposal is for -stdc++=c++98, not -std-c++=c++98 as the PR title suggests. Even you are confused about it just like I am :-) and my fingers hurt just trying to type it.

@thewilsonator
Copy link
Contributor Author

Whoops, that was originally going to be -std-c++= bit I forgot that trying to use that in cli.d's options caused OOMs that I didn't bother to track down, sorry for the confusion.

@WalterBright
Copy link
Member

The point is that's an all-too-easy mistake to make with this syntax, and we both botched it :-) Heck, I'll need a sticky note on my monitor to remember this one.

@thewilsonator
Copy link
Contributor Author

If you're really going to use it that much you should be putting it in your dmd.conf as with any other flags you use all the time. N.B. that it defaults to c++98 for the moment, that is expected to change after one stable release.

@WalterBright
Copy link
Member

dmd.conf is not a reason to use an unfortunate hard-to-remember switch. There's nothing wrong with -std=c++98. Nobody is going to be confused about it. Nobody is going to think it means Python98 or Java98.

I hate to say it, but if I showed -stdc++=c++98 during a presentation about how to use the compiler, people will laugh and I could not defend it.

@thewilsonator
Copy link
Contributor Author

#9029 (comment) point 3.

@WalterBright
Copy link
Member

WalterBright commented Dec 28, 2018

point 3:

My gripe with -std= is that it means "what language standard/dialect do we implement?". But we don't implement any part of C++ in the compiler, we only have an ABI compatibility layer.

-std=c++98 addresses it.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 28, 2018

Well, -stdc++=98 is only a slight improvement over -std=c++98, I was referring more towards not being fond of the std part of the option. I'll be looking for another way to express it when I merge this downstream.

@WalterBright
Copy link
Member

I don't see how -stdc++=98 is an improvement. It is rejected by g++. -std=c++98 is accepted by g++.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 28, 2018

And yet rejected by gcc, which instead accepts -std=c99. Then there's -std=f2018 that is only accepted by gfortran. In comparison, a D compiler might have an option like -std=dip25, which is to say: Conform to the DIP25 dialect of the D language spec.

The inclusion of an -std= option I feel should only refer to our own language. Compiler flags for controlling foreign languages should be appropriately prefixed.

My preference, however, is to not to call it by its current (still proposed?) name. In this PR, -std= is being used when you instead mean -abi=.

@thewilsonator
Copy link
Contributor Author

The inclusion of an -std= option I feel should only refer to our own language. Compiler flags for controlling foreign languages should be appropriately prefixed.

Indeed.

My preference, however, is to not to call it by its current (still proposed?) name. In this PR, -std= is being used when you instead mean -abi=.

I think some of the standard headers are versioned by #if __cplusplus >= whatever so its not strictly just ABI. This PR is the addition of the machinery for __cplusplus which is certainly not ABI, although in time more functionality (mangling ect.) will be controlled by this flag which is ABI.

If you can come up with a name that captures that, then please go ahead, it would be good to have consistency across the compilers.

@WalterBright
Copy link
Member

And yet rejected by gcc, which instead accepts -std=c99

It's still recognized by gcc, as it emits a message:

cc1: warning: command line option â-std=c++98â is valid for C++/ObjC++ but not for C [enabled by default]

The important thing about this is NOBODY is confused about c99 and c++98. There is no reason to have to type c++ twice. There is literally nothing better about -stdc++=c++98. It's redundant, and it is not compatible with any compiler's syntax.

Compiler flags for controlling foreign languages should be appropriately prefixed.

That's what the c++ part of c++98 does.

@WalterBright
Copy link
Member

If you can come up with a name that captures that

-cpp98

@ibuclaw
Copy link
Member

ibuclaw commented Dec 29, 2018

It's still recognized by gcc, as it emits a message:

That's due to the option handling being centralized in gcc. If you pass it a D option, you'll get a similar response. :-)

cc1: warning: command line option ‘-frelease’ is valid for D but not for C

I think some of the standard headers are versioned by #if __cplusplus >= whatever so its not strictly just ABI. This PR is the addition of the machinery for __cplusplus which is certainly not ABI, although in time more functionality (mangling ect.) will be controlled by this flag which is ABI.

It is an unfortunate mix where you have two different control flags. For example:

  • -std=c++98: It is not possible to have a function that uses nullptr_t, because the keyword is not recognized. Therefore, mangling typeof(null) as such becomes a question in D.
  • -std=c++11 -fabi-version=5: Here, nullptr_t is mangled and substituted (DnS_).
  • -std=c++11 -fabi-version=7: Here, nullptr_t is mangled and not substituted (DnDn).
  • -std=c++11 -fabi-version=9: Here, nullptr_t has a different alignment compared to prior versions of g++, how you pass typeof(null) from D suddenly becomes important.

@thewilsonator
Copy link
Contributor Author

It is an unfortunate mix where you have two different control flags.

Ouch! Well, I'll leave this to your best judgement. I've got no idea if clang has anything like, @JohanEngelen @kinke ?

@JohanEngelen
Copy link
Contributor

I don't know enough about clang's ABI mechanism to know for sure what all the complexities are. Clang does not have an -fabi-version= option or similar afaict. You can select the ABI in the triple, but there are only a few possibilities and there is not the amount of detail that e.g. GCC's -fabi-version=7 and -fabi-version=9 expose.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 29, 2018

Another example I can point to is Target.parameterSize, where dmd will return a different size when given an empty struct depending on OS - No, platforms don't have different ABIs, the underlying reason actually being gcc vs. clang.

Well, as of g++-8 (-fabi-version=12) gcc fixed the passing/returning of empty structs, so dmd/g++ ABI is now out of sync again after all the effort to fix it in the first place. :-)

This is slightly derailed from the original thread of discussion, and of course dmd doesn't get an easy ride as it tries to be compatible with all C++ compilers at the same time. The main point being however is that interfacing with C++ is not a simple binary answer (gcc or clang, c++98 or c++11), and you end up always being stuck with subtle ABI bugs because g++ is either too new or too old for dmd to support.

Anyway - and I've said this before - whatever ends up in the front-end, it should be accommodating enough to allow ldc to be compatible with clang and all its oddities, and gdc to be compatible with gcc in lock-step (plus plenty of oddities as well as I've already pointed out).

This is why I also point people to put things relating to the system in Target, where ldc and gdc can have full control over semantic and ABI related decisions.

This is not how you ask a target-specific question.

else if (ident == Id.wchar_t)
    tok = global.params.isWindows ? TOK.wchar_ : TOK.dchar_;

Instead, you should be phrasing it as:

else if (ident == Id.wchar_t)
    tok = target.tokenForCIdent(ident);

The above is perhaps poorly named, and has a questionable signature (it could be target.wchar_t_tok, or target.c_wchar_t_size), but you get the idea hopefully.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 29, 2018

I've already explained the problem that needs solving in gdc at large, and why I will be doing things slightly differently. As always, dmd is free to do what they want when it comes to the CLI. Having discrepancies between implementing D compilers is a problem solved by using dub. :-)

@WalterBright
Copy link
Member

There is another way. Simply support the latest g++/clang++ and whatever they do. If people can update their D compiler, they can also update their g++/clang++ compilers. Fortunately, that's free.

We don't have the resources to support every outdated variation of other compilers. I've chased these rainbows many times in the past, and it can easily get into a soul-sucking time-consuming grind, with little to no positive results.

@jacob-carlborg
Copy link
Contributor

There is another way. Simply support the latest g++/clang++ and whatever they do

It's not always that simple. It's possible to specify which version of C++ you want to use when compiling. Which version of C++ should we follow, the default when no flag is specified?

On macOS, Clang that is shipped with Xcode is usually a custom version of Clang built by Apple and is a snapshot of Clang upstream. For example, they use their own versions for the compiler. The latest version of Apple Clang still defaults to C++98 (or something like that). While, I think, latest upstream version of Clang has C++14 as the default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants