Skip to content

Add check in ts.h that language is C++, version 17 or later#10796

Merged
ywkaras merged 1 commit intoapache:masterfrom
ywkaras:ts_check_cpp17
Nov 27, 2023
Merged

Add check in ts.h that language is C++, version 17 or later#10796
ywkaras merged 1 commit intoapache:masterfrom
ywkaras:ts_check_cpp17

Conversation

@ywkaras
Copy link
Copy Markdown
Contributor

@ywkaras ywkaras commented Nov 16, 2023

No description provided.

@ywkaras ywkaras self-assigned this Nov 16, 2023
@ywkaras ywkaras added this to the 10.0.0 milestone Nov 16, 2023
@JosiahWI
Copy link
Copy Markdown
Contributor

JosiahWI commented Nov 16, 2023

This is also be enforced by CMake, but I guess it doesn't hurt to have a check in the source code.

@ywkaras
Copy link
Copy Markdown
Contributor Author

ywkaras commented Nov 16, 2023

This is also be enforced by CMake, but I guess it doesn't hurt to have a check in the source code.

It's possible someone might not want to use cmake to build their plugin.

@JosiahWI
Copy link
Copy Markdown
Contributor

This is also be enforced by CMake, but I guess it doesn't hurt to have a check in the source code.

It's possible someone might not want to use cmake to build their plugin.

Aha, this change makes sense now!

@ywkaras ywkaras requested a review from zwoop November 16, 2023 04:19
@maskit
Copy link
Copy Markdown
Member

maskit commented Nov 16, 2023

Would it cause errors if a plugin is built with C++ 11 where ATS itself is built with C++ 17?

We probably want to keep our right to use C++ 17 in ts.h, but I don't think we need to force plugin developers to use C++ 17 until we add something that requires C++ 17.

@ywkaras
Copy link
Copy Markdown
Contributor Author

ywkaras commented Nov 16, 2023

Do C++11 and C++17 have the same runtime library? Even if we are OK with the memory usage of having both loaded, will they have duplicate identifiers? Are there ABI issues?

@zwoop, do you want to change your mind about this?

https://www.open-std.org/JTC1/SC22/WG21/docs/papers/2018/p1319r0.html#removed

https://www.open-std.org/JTC1/SC22/WG21/docs/papers/2018/p0636r3.html#removed

Copy link
Copy Markdown
Contributor

@dragon512 dragon512 left a comment

Choose a reason for hiding this comment

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

GIven we we need c++ 17 or better. I think this is a wise option to be ABI working

@ywkaras ywkaras merged commit 820d0f9 into apache:master Nov 27, 2023
@bryancall bryancall mentioned this pull request Aug 14, 2024
91 tasks
@bryancall bryancall changed the title Add check in ts.h that language is C++, version 17 or later. Add check in ts.h that language is C++, version 17 or later Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants