flang-compiler / f18

F18 is a front-end for Fortran intended to replace the existing front-end in the Flang compiler
230 stars 48 forks source link

Remove use of std::set::merge #1014

Closed tskeith closed 4 years ago

tskeith commented 4 years ago

Some version of clang that we are building with don't have std::set::merge, even though it is part of C++17. Work around that by using std::set::insert until we can count on merge being available everywhere.

tskeith commented 4 years ago

I think that this is a case for conditional code so that the work-around only gets applied in cases where it's necessary, while the general case uses the standard API. Eventually the work-around case can be removed.

I don't know how to identify the cases where the work-around is needed. Some builds of clang 9 work and some don't, depending on the include files that are used.

jeanPerier commented 4 years ago

I think that this is a case for conditional code so that the work-around only gets applied in cases where it's necessary, while the general case uses the standard API. Eventually the work-around case can be removed.

I don't know how to identify the cases where the work-around is needed. Some builds of clang 9 work and some don't, depending on the include files that are used.

You could use CMake + try_compile to achieve this:

Pros of doing this: when all supported compiler should have this, we can have depreciation period before removing the macro and so on. I would only deploy all this if there is a performance hit of using insert vs merge though.

tskeith commented 4 years ago

Pros of doing this: when all supported compiler should have this, we can have depreciation period before removing the macro and so on. I would only deploy all this if there is a performance hit of using insert vs merge though.

Thanks for the idea -- that's a handy technique. I agree with you that we should use this only if there is a performance concern (and I'm pretty sure there isn't). It would add too much complexity for a temporary work-around.

But I'm not sure how to proceed. If we have to support building with C++ environments that don't have std::set::merge, I would propose keeping my change as-is. Otherwise, don't make any change. But then how do we describe the compiler requirements to someone who wants to build f18?

klausler commented 4 years ago

Can you make the change dependent on clang, independent of its version?

tskeith commented 4 years ago

Can you make the change dependent on clang, independent of its version?

Yes, I've put it in #ifdef __clang__.