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

Discussion about supporting C++17 #6585

Closed
2bbb opened this issue May 10, 2020 · 59 comments
Closed

Discussion about supporting C++17 #6585

2bbb opened this issue May 10, 2020 · 59 comments

Comments

@2bbb
Copy link
Contributor

2bbb commented May 10, 2020

Let's discussion about C++17.

@2bbb
Copy link
Contributor Author

2bbb commented May 10, 2020

#6488

@2bbb
Copy link
Contributor Author

2bbb commented May 10, 2020

@2bbb
Copy link
Contributor Author

2bbb commented May 10, 2020

on macOS early 10.14, we can't use C++17 with oF0.11.0 release, with both OF_USING_STD_FS 0 and OF_USING_STD_FS 1.
the biggest cause of this problem is by Apple LLVM/clang.

but essentially, I think it lies under extending std namespace adding filesystem.
extending std is so evil way.

I fixed bug about difference between std::filesystem and std::experimental::filesystem.
but it doesn't fix problem on macOS early 10.14.

and if try to use C++17 on macOS 10.14, then we need to remove boost::filesystem from std with OF_USING_STD_FS 0 or adding so many CPP macros and separating codes.

I want to propose what adding of (or other name) namespace and add boost::filesystem or std::experimental::filesystem or std::filesystem.
it still needs some macros to absorb the diff of some APIs, but doesn't need to adding macros.

what do you think?

@roymacdonald
Copy link
Member

Hey @2bbb I think that this move to support C++17 will eventually happen so we should consider it right now how to address it. I think that it is important that these changes don't alter the way in which of is used in general, as far as this is possible and does not bloat the codebase or needs the use of hacky fixes.
Can you post the code changes that you are proposing?
best!

@oxillo
Copy link
Contributor

oxillo commented May 17, 2020

Do we still need to use boost::filesystem in C++17 (meaning C++17 and OF_USING_STD_FS=0) ?
I would say that most people using C++17 will expect to use std::filesystem.
So I would suggest that OF_USING STD_FS is only applicable to C++14 and below.
This will also make solving #6587 a lot easier...

@2bbb
Copy link
Contributor Author

2bbb commented May 20, 2020

@roymacdonald

i will push to my repository asap.

@oxillo
basically, i agree to what you say.
but, on older macOS (~Mojave) has C++17 option but can't use std::filesystem::path...
it still needs option to use boost::filesystem.

@arturoc
Copy link
Member

arturoc commented May 23, 2020

boost was only used temporarily until there was filesystem support in std.

The idea is not to keep it but to remove it as soon as we can and stop even shipping the boost libraries.

This needs to happen gradually by first only enabling OF_USING_STD_FS for those compilers that already have full support for c++17 then after the minimum version supported by OF already supports c++17 fully remove boost completely

We would need to identify what versions of the supported compilers already support c++17 fully a way to detect the version.

I would say:

  • Windows visual sutdio: already supported and since we only support the latest version we can already even remove boost from OF
  • Windows msys2: is now at gcc10 so we can also already remove boost
  • MacOS / iOS: we need a way to figure out the version in use and enable boost or std fs accordingly
  • Linux (including arm): detect gcc 10 or greater in the makefiles and enable OF_USING_STD_FS there based on that
  • Android: no idea and in any case we support a pretty old version of the NDK so we surely need to just stick to boost by now although we could also enable it through experimental which is also supported when enabling OF_USING_STD_FS

@oxillo
Copy link
Contributor

oxillo commented May 27, 2020

I had a bad experience last year when enabling OF_USING_STD_FS on msys2 and running the tests. It deleted my OF dir...
I've managed to compile OF on MSYS2/GCC10 with the following 3 configurations :

  • gnu++14/OF_USING_STD_FS=0
  • gnu++14/OF_USING_STD_FS=1
  • c++17

In ofConstants.h, I've added the c++17 detection and removed the filesystem #if
In ofFileUtils.h, I've rewritten the #if section as :

#if HAS_CPP17 // Use std::filesystem
	#include <filesystem>
#else
	#if OF_USING_STD_FS // Use std::experimental::filesystem
		#include <experimental/filesystem>
		namespace std {
			namespace filesystem = experimental::filesystem;
		}
	#else // Use boost::filesystem
		#if !_MSC_VER
			#define BOOST_NO_CXX11_SCOPED_ENUMS
			#define BOOST_NO_SCOPED_ENUMS
		#endif
		#include <boost/filesystem.hpp>
		namespace std {
			namespace filesystem = boost::filesystem;
		}
	#endif // OF_USING_STD_FS
#endif //HAS_CPP17

I'll try to run the tests now after a backup...

@kaofishy
Copy link

BTW, boost::filesystem and std::filesystem are not drop-in replacement of each other, last time I tried to get OF_USING_STD_FS to work (it was a while ago) I encountered type definition mismatch and I recall particular difficulty to get std::filesystem::path to work with existing openFrameworks code.

See this post for differences between boost::filesystem and std::filesystem.

@arturoc
Copy link
Member

arturoc commented May 28, 2020

Yes they are not exactly the same but OF_USING_STD_FS not only includes one or the other but also ifdefs certain calls that are different for both. Exisiting code might need to be changed when directly using std::filersystem

There might be some differences from std::filesystem experimental and the finall c++17 implementation. I think i only tested it with experimental when implementing it so there might be some changes that need to be made to get it to work with c++17

@2bbb
Copy link
Contributor Author

2bbb commented Jun 8, 2020

https://github.com/2bbb/openFrameworks/tree/dev/supporting_cpp17

codes in this branch is better way i think.
(this branch includes #6579 what fix different on std and std::expermental)

this changes of code make another namespace of (i can't know this name is good or bad. but it is not current problem.), and include filesystem from std or std::experimental or boost.

i know this will break codes using std::filesystem.
and also, i know oF codes have not made namespace explicitly. (i know there is ofx namespace in detail of codes.)

but i think, this is better way.
because, in any case, we cannot use filesystem uniformly across some envs because some difference be in three filesystems.
and i think, to hide these difference makes someone confusing when use C++17.

if we will not support old envs, then perhaps we can choose other way.
but if we will support old envs partialy support std::filesystem, then we need to use boost::fileystem with partial C++17 envs. and it conflicts about filesystem.

sorry to my not better explanation.
i believe you understand what i want to say.

thanks.

@artificiel
Copy link
Contributor

as a user i'd like to stimulate this issue — the real subject here is not "supporting c++17" but solving the ::filesystem ambiguity.

until recently i was able to compile with c++17 (macOS and linux) but because of a compiler update the ::filesystem thing forces me to specifying down to c++14 and having adjust some things and to pepper #pragmas so the compiler does not flood the output with warnings (while still supporting the features).

"transparent" c++17 usage would be very much appreciated! (and more and more 3rd party libs and example code are relying on c++17).

@ofTheo
Copy link
Member

ofTheo commented Jul 10, 2020

As far as detecting if the compiler supports c++17 and std::filesystem this macro is defined when filesystem is available
__cpp_lib_filesystem and this one is defined when experimental is available __cpp_lib_experimental_filesystem

So we could probably use these across all compilers to get the right level of fallback.

I could work on a PR that handles the fallback and also check to see if the final spec calls are 1:1 replacements of the experimental.

see:
https://en.cppreference.com/w/User:D41D8CD98F/feature_testing_macros#C.2B.2B17
and:
https://stackoverflow.com/questions/53365538/how-to-determine-whether-to-use-filesystem-or-experimental-filesystem

@arturoc
Copy link
Member

arturoc commented Jul 10, 2020

@ofTheo if you are going to look into this, take into account that everything is mostly done all the differences between apis are already ifdefd and there's macros: OF_USING_STD_FS to enable one or the other. this was all working around a year ago but probably some final change in the final standard macros have broken the detection. it should be just a matter of changing a coupld of macros in ofFileUtils.h everything else should just work after that unless there's been more changes to the actual filesystem api but i doubt that's the case

@ofTheo
Copy link
Member

ofTheo commented Jul 10, 2020 via email

@RodenLuo
Copy link

RodenLuo commented Oct 11, 2020

Hi, How is the progress here? Is it possible now to use openFrameworks in c++17 on Mac? If I just change the compiler version and leave everything else as-is, I got a lot of the Reference to 'filesystem' is ambiguous error. Thanks.

of_v0.11.0_osx_release
Xcode: Version 12.0.1 (12A7300)
Mac OS: 10.15.7 (19H2)
C++17 [-std=c++17]

@2bbb
Copy link
Contributor Author

2bbb commented Oct 11, 2020

@RodenLuo
if you use macOS 10.15.X, you can use C++17 on macOS.

  • set macOS Deployment Target 10.15
  • change OF_USING_STD_FS to 1 on ofConstants.h
  • fix about some std::filesystem::permissions on ofFileUtils.h like this

app with this modifications only works on macOS 10.15~.

@RodenLuo
Copy link

Hi @2bbb
Thanks! I did exactly the same, but not working.

With GNU++17, I get the following errors:
image

With C++17, I get the following errors:
image

If more details are interesting and needed, I can share the compiling log here. I noticed that there are some changes for ofConstants.h in the given link. I then made those changes as well. But no luck. Similar errors as above.

@2bbb
Copy link
Contributor Author

2bbb commented Oct 11, 2020

@RodenLuo
did you change Deployment Target & C++ version in libs/openFrameworksCompiled/project/osx/openFrameworksLib.xcodeproj?

@RodenLuo
Copy link

Thanks! I just changed that as well. It does not work on my project as there are errors coming from the ofxGui I used. But it does work on an empty project.

Sorry that in my last post I only changed Deployment Target & C++ version in my project.

@artificiel
Copy link
Contributor

poking this again... anything happening "behind the scenes" on this front? it is getting trickier as external libs are increasingly relying on C++17. it would be super great to be able to just have to specify -std=c++17 and have things running coherently.

@themancalledjakob
Copy link
Contributor

themancalledjakob commented May 2, 2021

.... it would be super great to be able to just have to specify -std=c++17 and have things running coherently.

Speaking for Linux and gcc... could it actually be as easy as just doing that?

Is there something wrong with this approach to use -std=c++17 when gcc version >=7?:

--- a/libs/openFrameworksCompiled/project/makefileCommon/config.linux.common.mk
+++ b/libs/openFrameworksCompiled/project/makefileCommon/config.linux.common.mk
@@ -133,10 +133,12 @@ PLATFORM_REQUIRED_ADDONS =
 # < 4.7.x  c++0x
 # >= 4.7.x c++11
 # >= 4.9.x c++14
+# >= 7.x.x c++17
 # other compilers c++11 by now
 ifeq ($(CXX),g++)
        GCC_MAJOR_EQ_4 := $(shell expr `gcc -dumpversion | cut -f1 -d.` \= 4)
        GCC_MAJOR_GT_4 := $(shell expr `gcc -dumpversion | cut -f1 -d.` \> 4)
+       GCC_MAJOR_GT_7 := $(shell expr `gcc -dumpversion | cut -f1 -d.` \> 7)
        GCC_MINOR_GTEQ_7 := $(shell expr `gcc -dumpversion | cut -f2 -d.` \<= 7)
        GCC_MINOR_GTEQ_9 := $(shell expr `gcc -dumpversion | cut -f2 -d.` \>= 9)
        ifeq ("$(GCC_MAJOR_EQ_4)","1")
@@ -153,9 +155,14 @@ ifeq ($(CXX),g++)
                        endif
                endif
        endif
-       ifeq ("$(GCC_MAJOR_GT_4)","1")
+       ifeq ("$(GCC_MAJOR_GT_7)","1")
                PLATFORM_CFLAGS = -Wall -Werror=return-type -DGCC_HAS_REGEX
-               PLATFORM_CXXFLAGS = -Wall -Werror=return-type -std=c++14 -DGCC_HAS_REGEX
+               PLATFORM_CXXFLAGS = -Wall -Werror=return-type -std=c++17 -DGCC_HAS_REGEX
+       else
+               ifeq ("$(GCC_MAJOR_GT_4)","1")
+                       PLATFORM_CFLAGS = -Wall -Werror=return-type -DGCC_HAS_REGEX
+                       PLATFORM_CXXFLAGS = -Wall -Werror=return-type -std=c++14 -DGCC_HAS_REGEX
+               endif
        endif
 else
        ifeq ($(CXX),g++-5)

I am currently running the testAllExamples.sh script, and it seems to all work so far. Except I don't have a webcam or speakers at the moment, so I cannot test these examples properly. But I don't really see why there would be an issue.
Also, I could successfully use c++17 libraries... Is there a reason why this might not be a good idea?

Tested on Ubuntu 20.04 and Debian 10.9

@ofTheo ofTheo mentioned this issue Sep 3, 2021
77 tasks
@austin-clifton
Copy link

austin-clifton commented Oct 3, 2021

Hi all, I was able to get the Visual Studio 2017 build working with c++17 with some really minimal changes off of master branch thanks to @2bbb 's recent fixes to the fs utils.

I didn't realize I'd make a reference this way across forks, but the commit is above (EDIT: below now, left a comment for why moving using namespace std; was necessary, sorry for the double commit reference). If the C++ standard is C++14 it will build with OF_USING_STD_FS=0, if the standard is C++17 it will build with OF_USING_STD_FS=1. Should OF_USING_STD_FS be set based on the value of __cplusplus instead?

The only gotcha is you must add /Zc:__cplusplus to the "Configuration Properties" --> "C/C++" --> "Command Line" --> "Additional Options" section of any generated projects (Visual Studio will set the __cplusplus macro to C++98 otherwise, see here). I would guess that could be added to the project generator?

@roymacdonald
Copy link
Member

I would guess that could be added to the project generator?
Project generator takes the templates from scripts/templates/vs and adds the necessary stuff to it in order to generate a new project. Thus, if you modify that you should be able to add the needed options. Although, maybe it is a good idea to make a separate template for C++17, so it does not break currently working stuff. I think that this will not reflect immediately in the Project Generator, thus some code needs to be added to it.

@ofTheo
Copy link
Member

ofTheo commented Oct 5, 2021

Just a note that C++17 would mean dropping macOS 10.14 from supported platforms.
I think typically we have supported the last 2-3 macOS platforms.

With macOS 12 coming out this Autumn it could be fair to drop 10.14 ( though that is still what I use ).

I would be curious about whether it could be worth making OF 0.12.0 support only macOS 10.15 and up for the benefit of having C++17 on by default.

yays/nays? 🙂

@roymacdonald
Copy link
Member

@ofTheo but why we would need to drop support for 10.14 (it is what I use too)?. Cant we support both and add some stuff to PG so it can automatically decide which version to use depending on your OS?

@ofTheo
Copy link
Member

ofTheo commented Oct 6, 2021 via email

@dimitre
Copy link
Member

dimitre commented Dec 5, 2021

Another nice possibility with OFW using C++17
https://developer.apple.com/metal/cpp/

@dimitre
Copy link
Member

dimitre commented Jan 10, 2022

Maybe it already deserves its own branch with changes from @2bbb and @themancalledjakob ?

@ofTheo
Copy link
Member

ofTheo commented Jan 19, 2022

I would love to get the needed changes merged in soon for this.

I'll use this issue as a reference but if anyone has working C++17 branches they are using it would be a big help if you could link to them here.

Thanks!

@themancalledjakob
Copy link
Contributor

these work on linux for me. (tested on Debian 10/11 & Ubuntu 20.04)
https://gitlab.com/pointerstudio/utils/openframeworks/-/tree/feature-cpp17
https://gitlab.com/pointerstudio/utils/projectGenerator/-/tree/feature-cpp17

the commits could be cleaner though, and of course Windows & Mac are ignored.
I'd be happy to fork and PR if some if it could be helpful.

@ofTheo
Copy link
Member

ofTheo commented Jan 20, 2022

Thanks @themancalledjakob and everyone on this thread!

For macOS I was able to get c++17 and std::filesystem working by only modifying the xcconfig of an example project and slightly tweaking the compile OF build phase.

Starting with the current master or the nightly builds on the OF download page:

1. Add this to your Project.xcconfig ( Note: it would probably ship commented out )

//UNCOMMENT BELOW TO ENABLE C++ 17 and std::filesystem 
CLANG_CXX_LANGUAGE_STANDARD = c++17
MACOSX_DEPLOYMENT_TARGET = 10.15
GCC_PREPROCESSOR_DEFINITIONS = OF_USING_STD_FS=1

Screen Shot 2022-01-20 at 3 38 24 PM

2. Change the first Run Script in Build Phases to:

xcodebuild -project "$OF_PATH/libs/openFrameworksCompiled/project/osx/openFrameworksLib.xcodeproj" -target openFrameworks -configuration "${CONFIGURATION}" GCC_PREPROCESSOR_DEFINITIONS=$GCC_PREPROCESSOR_DEFINITIONS CLANG_CXX_LANGUAGE_STANDARD=$CLANG_CXX_LANGUAGE_STANDARD MACOSX_DEPLOYMENT_TARGET=$MACOSX_DEPLOYMENT_TARGET

Screen Shot 2022-01-20 at 3 29 39 PM

What this does is allow the Project.xcconfig settings to overwrite the OFCore.xcconfig settings so when OF is compiled by script in the Run Script phase it uses the C++ version from Project.xcconfig

@themancalledjakob I didn't end up needing to add the #include ofFileUtils.h to those three files but I will keep those if they are needed on linux.

I will start a PR in a sec with these changes and then work on pulling the changes needed for Linux and Windows.

@ofTheo
Copy link
Member

ofTheo commented Jan 21, 2022

Here is the branch:
https://github.com/ofTheo/openFrameworks/tree/branch-cpp-17

Compare to see the changes so far:
https://github.com/openframeworks/openFrameworks/compare/master...ofTheo:branch-cpp-17?expand=1

Right now it supports cpp 17 for macOS only.
Will work on macOS makefile next then Windows.

Happy if people want to PR to that branch for other platforms.
If our minimum platforms support C++17 feel free to make that enabled by default.

Once it is all working via CI I'll PR the main repo.

@ofTheo
Copy link
Member

ofTheo commented Jan 21, 2022

Okay I can get similar functionality with the macOS makefiles.
ofTheo@f1c4670

Though in order to be able to do C++17 and std::filesystem on a per project basis I use this approach. Curious what thoughts are on this.

In a project or examples config.make I add this to the end:

export MAC_OS_MIN_VERSION = 10.15
export MAC_OS_CPP_VER = -std=c++17
export PLATFORM_DEFINES_EXTRA = OF_USING_STD_FS=1

In the config.osx.default.mk I change:

PLATFORM_DEFINES = __MACOSX_CORE__

to

PLATFORM_DEFINES = __MACOSX_CORE__

# Allows a projects config.make to pass in extra defines for the core and project 
#ifdef PLATFORM_DEFINES_EXTRA
	PLATFORM_DEFINES += $(PLATFORM_DEFINES_EXTRA)
#endif

and then add this so we can override the std cpp version:

ifndef MAC_OS_CPP_VER
    MAC_OS_CPP_VER = -std=c++11
endif

This results in the project and the core being built with c++17 and std::filesystem.
I sort of like this approach so far as it means that the project generator could easily set c++17 and std::filesystem on a per project basis.

Curious though if anyone sees anything problematic with the above?
The export seems a bit iffy but otherwise I am not sure there is a better way to get stuff from a projects config.make to config.osx.default.mk?

@ofTheo
Copy link
Member

ofTheo commented Jan 22, 2022

macOS and VS are working now in PR #6844 with a simpler approach than above.
I think for most platforms all that is needed now is setting -std=c++17 for OF and the Project.

Feel free to PR this branch with other platforms:
https://github.com/ofTheo/openFrameworks/tree/branch-cpp-17

@oxillo would you be able to take a look at msys2 ?

@ofTheo
Copy link
Member

ofTheo commented Mar 10, 2022

closed by #6844

@ofTheo ofTheo closed this as completed Mar 10, 2022
@2bbb
Copy link
Contributor Author

2bbb commented Mar 10, 2022

🎉

@dimitre
Copy link
Member

dimitre commented Mar 10, 2022

WOWWW great work @ofTheo

@kaofishy
Copy link

Amazing work! Thank you @ofTheo

@roymacdonald
Copy link
Member

BTW, I still use macOS 10.14.6 with xcode 11.2.1, with Xcode 11.2.1. If I change the xcode project settings to set the deploy target to 10.14 and C++ version to C++14 it compiles and works fine. Just in case any other wants to still use Macos Mojave
Screen Shot 2022-04-18 at 22 34 15
Screen Shot 2022-04-18 at 22 34 35
.

@stungeye
Copy link

Very excited to have C++17 with openFrameworks. Great work @ofTheo! 👏

@himwho
Copy link

himwho commented Jun 2, 2022

I just wanted to see if there were any ideas on how to best use C++17 (needed for one ofx that requires std::optional) but to still support mojave (10.14) by somehow changing std::filesystem for 10.14 users?

I have things working now but with the minimum target of 10.15 I am getting some angry users... ;)

@ofTheo
Copy link
Member

ofTheo commented Jun 2, 2022

@himwho I am actually working a bit more on this right now as we're not catching al the nuances with linux and C++ filesystem right now.

For your situation I just tried an idea with the nightly builds ( at the bottom of the OF download page ) where we enable boost::fs and C++17, but then I get the ambiguous reference to filesystem error. error: reference to 'filesystem' is ambiguous as both boost defines it and C++17 defines it

If there is a way to resolve that then this is what I did:

  1. In the main project's Project.xcconfig
    Change deployment target to 10.14.

  2. Then change the last line of:
    libs/openFrameworksCompiled/project/osx/CoreOF.xcconfig to: GCC_PREPROCESSOR_DEFINITIONS = $(inherited) GL_SILENCE_DEPRECATION=1 GLES_SILENCE_DEPRECATION=1 OF_USING_STD_FS=0

I'm not sure if there is a way to have C++17 and declare namespace filesystem = boost::filesystem; ( in ofConstants.h )
If you can figure it out please update this issue.

@himwho
Copy link

himwho commented Jun 3, 2022

@ofTheo I am a little out of the loop when it comes to the switch from boost::filesystem to std::filesystem, i just did a test where I forced usage of boost::filesystem throughout OF while setting the target deployment to 10.14 and setting the project to use c++17 and this built fine, my immediate reaction is why not stick with boost? especially if its supported both new and old macOS wise?

Beyond that I guess I will look into how to elegantly use std namespace when 10.15+ and use boost namespace when 10.14 or below simply to handle support for filesystem::path which seems to be the only sore thumb in the situation. Any recommendations there would be appreciated as this is not something I have a ton of experience with.

I guess where i need to wrap my head is do I really want to support and package both and figure out how to test the client for which OS version they have to switch between the two?

@ofTheo
Copy link
Member

ofTheo commented Jun 3, 2022

hmm @himwho - I am surprised you didn't get the error: reference to 'filesystem' is ambiguous as I had 10.14 as the target C++17 and boost filesystem selected and got the error above.

If it did work then you could deploy that app to both 10.14 and 10.15+ systems.

Was this with the 0.11.2 release or the nightly builds?

Thanks!
Theo

@himwho
Copy link

himwho commented Jun 3, 2022

@ofTheo it was with latest OF on git commit , but just to make sure its clear i literally redeclared every usage of std::filesystem as boost::filesystem effectively ignoring std::filesystem

I havent tested on target devices yet but just was happy it compiled with target 10.14 and ran on my dev machine running 11.6

@ofTheo
Copy link
Member

ofTheo commented Jun 3, 2022

Ah okay - yeah I think that is the issue as we are defining boosts file system to std::filesystem eg:
https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/utils/ofFileUtils.h#L290

If we has a different argument or namespace like removeExt(const of::filesystem::path& filename) then we could more easily switch between implementations - as if we have both

namespace std { namespace filesystem = boost::filesystem; }

and

std::filesystem via C++17 the compiler doesn't know which one to choose.

There is some interesting info on the detection stuff here:
https://stackoverflow.com/questions/53365538/how-to-determine-whether-to-use-filesystem-or-experimental-filesystem

About to open a PR to fix the C++17 fs detection on linux and will link it here for reference.

@ofTheo
Copy link
Member

ofTheo commented Jun 3, 2022

#7013 PR with some cpp17 filesystem fixes

@himwho
Copy link

himwho commented Jun 6, 2022

@ofTheo is there a clever way to detect which macOS version is available on runtime?

#ifndef OF_USING_STD_FS
	#if OF_HAS_CPP17 && [macos_10.15+]
        	#define OF_USING_STD_FS 1
	#else
		// Set to 1 to force std filesystem instead of boost's
		#define OF_USING_STD_FS 0
	#endif
#endif

we just really need to fallback to boost if <10.14 and otherwise all the conditionals you did were good to go
does it make more sense to follow this advice?

If you're feeling very fancy, you can use the __has_include define to check if the standard filesystem include is available, and if so, set namespace fs = std::filesystem and fallback to Boost only if the std version isn't available.

@ofTheo
Copy link
Member

ofTheo commented Jun 6, 2022

@himwho the problem with all the preprocessor stuff like __has_include and OF_USING_STD_FS etc is that it is set not based on the machine running the app but the machine building it. So if you build it on 10.15 and deploy it on 10.14 it will try and use the features that were available to the 10.15 compiler.

I think for your case you would simply want it to use the boost filesystem no matter what, but as mentioned above for that to work we would need to replace all the instances of std::filesystem arguments with a different namespace like: of::filesystem so we don't get the ambiguous errors.

eg:

ofFileUtils:::removeExt(const of::filesystem::path& filename); 

This might be good practice during transitions like this but curious what others think?

@himwho
Copy link

himwho commented Jun 6, 2022

@ofTheo
yeah i was toying with that idea already: 63119b9

also I expanded the conditional to catch my case: 63119b9#diff-67461da5aefff862c534547a36c8a8bbfd65fd0985c30f9a69d21a3a0fd815a4L456

        // if target includes version 10.14 or below
        #if(MAC_OS_X_VERSION_MIN_REQUIRED <= MAC_OS_X_VERSION_10_14)
            #define OF_USING_STD_FS 0
        #else
            #define OF_USING_STD_FS 1
        #endif

i opted for fs::path&, but does this raise any concerns with any ofx addons that are still declaring a specific flavor of filesystem?

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

No branches or pull requests