Fix compilation on compilers which do not have Atomic built-in
ClosedPublic

Authored by kiroma on Oct 13 2018, 8:21 PM.

Details

Diff Detail

Repository
R37 Krita
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kiroma created this revision.Oct 13 2018, 8:21 PM
Restricted Application added a reviewer: Krita. · View Herald TranscriptOct 13 2018, 8:21 PM
Restricted Application added a project: Krita. · View Herald Transcript
kiroma requested review of this revision.Oct 13 2018, 8:21 PM
kiroma edited the summary of this revision. (Show Details)Oct 13 2018, 8:25 PM
rempt added a subscriber: rempt.Oct 14 2018, 8:49 AM
rempt added inline comments.
libs/image/CMakeLists.txt
332

Please don't push unrelated changes.

338

Shouldn't that then also check for the clang version, or is this safe?

With cases like this it's usually preferred to do feature detection instead of checking for specific versions.

You should add a check with CMake to test whether a simple test program can be compiled with/without linking libatomic, or use an existing CheckAtomic.cmake from an appropriate source.

kiroma planned changes to this revision.Oct 14 2018, 1:38 PM

Following Alvin, I don't think there's any CheckAtomic.cmake written yet, so I'll make a new one.

kiroma updated this revision to Diff 43596.Oct 14 2018, 5:03 PM

Turns out there is a CheckAtomic.cmake already created, in the LLVM project. I don't know how to handle licensing, I've never done it before, so I included the license in the file copied over from LLVM.

kiroma marked an inline comment as done.Oct 14 2018, 5:25 PM
kiroma added inline comments.
libs/image/CMakeLists.txt
338

It is standard for all clang versions to require linking against atomic if it's used.
However I'm going to use a more universal approach.

kiroma marked 2 inline comments as done.Oct 14 2018, 5:25 PM
kiroma retitled this revision from Fix compilation with clang-7 to Fix compilation on compilers which do not have Atomic built-in.Oct 14 2018, 9:28 PM
rempt accepted this revision.Oct 15 2018, 2:23 PM

Do you have push access?

This revision is now accepted and ready to land.Oct 15 2018, 2:23 PM

Nope, first patch ever.

rempt added a comment.Oct 16 2018, 7:21 AM

Okay, I'll push it today :-)

This revision was automatically updated to reflect the committed changes.
kiroma reopened this revision.Nov 20 2018, 11:43 AM

Oh hey I noticed this patch was reverted, can I get some pointers to where it failed so I can fix it up?

This revision is now accepted and ready to land.Nov 20 2018, 11:43 AM

Aw, zut, for some reason my comment didn't get posted. I had to revert it because of https://bugs.kde.org/show_bug.cgi?id=399953. The patch broke building with clang on macOS and freebsd on KDE's jenkins.

dkazakov requested changes to this revision.Dec 25 2018, 7:49 AM
dkazakov added a subscriber: dkazakov.

Since the patch was reverted and breaks something, I'm marking it as Needs Changes

This revision now requires changes to proceed.Dec 25 2018, 7:49 AM
kiroma updated this revision to Diff 49337.Jan 12 2019, 3:54 PM

cmake now checks if neither atomic nor atomic64 is compilable without linking in atomics, tested out on FreeBSD, should work on MacOS.

Alright so this patch was long overdue to be fixed, considering how simple the fix turned out to be.
I've tested it out on FreeBSD under KVM and it compiled just fine.
I haven't yet ran all tests, I'll send another message when I deem the patch ready to merge.

Alright well there are unrelated errors but everything else seems to work fine.

rempt added a comment.Jan 15 2019, 9:31 AM

Yay! It works now!

This revision was not accepted when it landed; it landed in state Needs Review.Jan 15 2019, 9:33 AM
This revision was automatically updated to reflect the committed changes.
kiroma edited the summary of this revision. (Show Details)Jan 16 2019, 12:48 PM