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

[C++] Bump minimum required version to C++17 #3335

Merged
merged 2 commits into from
Nov 29, 2021
Merged

Conversation

jcking
Copy link
Collaborator

@jcking jcking commented Nov 1, 2021

Bump the minimum required version to C++17 and remove preprocessor guards that are no longer necessary. Additionally it switches antlrcpp::SingleWriteMultipleReadLock to just use std::shared_mutex. #3330

@jcking jcking force-pushed the cpp17 branch 2 times, most recently from 73ca812 to 37c1502 Compare November 1, 2021 14:04
@jcking
Copy link
Collaborator Author

jcking commented Nov 1, 2021

@mike-lischke

@mike-lischke
Copy link
Member

Good job! Though we need a look at Visual Studio Code and Xcode. Can you do that yourself? I might be able to check Xcode, however.

@jcking
Copy link
Collaborator Author

jcking commented Nov 3, 2021

I do not have a Mac to check XCode. What in visual studio code needs to be checked? I do not see any of the editor configuration files directly checked in. Do you mean Visual Studio?

@mike-lischke
Copy link
Member

Ah, I'm working too much in vscode lately :-) I meant Visual Studio of course. I don't expect much to be changed (if at all) for VS, as that by default uses the latest support C++ dialect. In Xcode we need to change at least the build project settings for C++17. Not sure when I'll find the time to do that, but I see what I can do.

@jcking
Copy link
Collaborator Author

jcking commented Nov 4, 2021

I think I got them, but I am not sure if its correct as it was manual. I also fixed the files after the guid.{h,cpp} -> Guid.{h,cpp} rename.

@mike-lischke
Copy link
Member

mike-lischke commented Nov 14, 2021

Strange, I just committed my macOS fixes to this PR and was successful, but the changes don't appear here. Is there anything else that must be done?

Note: I checked out the branch using gh pr checkout 3335.

@jcking
Copy link
Collaborator Author

jcking commented Nov 15, 2021

That might be because I force pushed at some point after fixing something. If you still have them locally you can pull the latest changes for the pr then repush them.

@mike-lischke
Copy link
Member

My changes still exist (https://github.com/mike-lischke/antlr4/tree/cpp17) and I have merged in all other changes, but they don't automatically merge, as they should. I guess I have to open a new PR with them. If you don't have a better idea I'll ask Ter to merge this PR and I'll open a follow up PR then.

@jcking
Copy link
Collaborator Author

jcking commented Nov 28, 2021

Works for me.

@jcking
Copy link
Collaborator Author

jcking commented Nov 28, 2021

@parrt

@parrt
Copy link
Member

parrt commented Nov 28, 2021

Hi @jcking ! Upgrading to C++17 sounds great, assuming most compilers can handle it, but I'll have to defer to @mike-lischke thanks!

@parrt parrt added this to the 4.9.4 milestone Nov 28, 2021
@mike-lischke
Copy link
Member

OK, then let's do it. @parrt please merge this patch. I'll open a new PR with my additional changes.

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

Successfully merging this pull request may close these issues.

3 participants