Project

General

Profile

Bug #11647

Patches for C++14 on Clang

Added by Ben Morgan over 4 years ago. Updated about 4 years ago.

Status:
Closed
Priority:
Normal
Target version:
Start date:
02/05/2016
Due date:
% Done:

100%

Estimated time:
Spent time:
Duration:

Description

The attached patches remove a tiny bit of the Boost dependency and fix several compilation errors of cetlib on OS X with Clang (Apple's Clang from Xcode 7, so clang-3.6/7ish).
Testing is not complete across all platforms, but tests related to the patched areas should compile and pass. Compilation was used with a local patched version of cetbuildtools that allows
the native compiler to be used - flags were confirmed to be the same as applied to GCC (the branch with this is available on GitHub: https://github.com/drbenmorgan/fnal-cetlib/tree/feature/modern-cmake)

Commit messages in the patches describe the issue and resolution.

Note that patch 0005 is related to Issue #6951, which re-occured as the patch supplied in that issue was not applied fully.

Associated revisions

Revision a934cdcc (diff)
Added by Chris Green over 4 years ago

Apply 0001-Use-std-array-in-place-of-boost-array.patch from issue #11647.

Revision 34650127 (diff)
Added by Chris Green over 4 years ago

Apply 0002-Don-t-use-temp-regex-in-sregex_token_iterator.patch from issue #11647.

Revision 6e9dfd40 (diff)
Added by Chris Green over 4 years ago

Apply 0004-cpu_timer_test-explicitly-include-cmath.patch from issue #11647.

Revision 41a4b28c (diff)
Added by Chris Green over 4 years ago

Apply 0005-Apply-__clang__-blocks-consistent-with-GCC-Intel.patch from issue #11647.

Revision 7380bc67 (diff)
Added by Chris Green over 4 years ago

Apply 0004-cpu_timer_test-explicitly-include-cmath.patch from issue #11647.

Revision 1b481446 (diff)
Added by Chris Green over 4 years ago

Apply 0005-Apply-__clang__-blocks-consistent-with-GCC-Intel.patch from issue #11647.

Revision 3ab6fe4f (diff)
Added by Chris Green over 4 years ago

Revert "Apply 0001-Use-std-array-in-place-of-boost-array.patch from issue #11647."

This reverts commit a934cdccdd50aa7ca0996f79a7e19210f9397baa.

Using std::array here affects persistency with ROOT6. This was apparently hidden in earlier tests due to a build hysteresis and was exposed with a clean build.

History

#1 Updated by Kyle Knoepfel over 4 years ago

  • Status changed from New to Accepted

Thank you. We will review each patch and apply as appropriate.

#2 Updated by Ben Morgan over 4 years ago

Kyle Knoepfel wrote:

Thank you. We will review each patch and apply as appropriate.

Thanks. As a quick addendum to "0003-Fully-qualify-cet-namespace-for-cbegin-cend.patch", I've since discovered that this does not fix the issue for clients of cetlib as the tests do not completely cover the functionality of source:cetlib/container_algorithms.h. Errors are subsequently seen when trying to build fhicl-cpp as this uses functions in container_algorithms that cetlib doesn't test (and thus instantiate).

#3 Updated by Kyle Knoepfel over 4 years ago

  • Assignee set to Christopher Green

#4 Updated by Christopher Green over 4 years ago

Your contributed patches have been applied, with the exception of 0003-Fully-qualify-cet-namespace-for-cbegin-cend.patch. We believe the latter goes too far in the direction of breaking a recommended C++ idiom in order to accommodate broken and non-broken compilers (GCC < 5.1 and Clang >=3.6, respectively). Please see counter-proposal attachment:0003-Alternative-to-0003-Fully-qualify-cet-namespace-for-.patch, which makes the accommodation for broken compilers explicit with minimal disruption to the best practice of unqualified use of cbegin() and cend() to provide maximum flexibility when allowing for different (possibly non-STL) containers.

Please try the proposed code out against clang (if you care about older Clang compilers, please feel free to adjust the terms of the macro definition in source:cetlib/compiler_macros.h) and let me know if any changes are required.

Note that this is a breaking change: if you are explicitly referring to cet::cbegin() or cet::cend() anywhere in your own code, you will need to change to using the recommended idiom, which will obviate the need for further changes (except for optional cleanup) when all compilers in use support std::cbegin() and std::cend().

For completeness, the recommended idiom is:

{
  use namespace std;
  use namespace cet::container_helpers;
  // ... use unqualified cbegin(), cend(), etc.
}

Commits:

*   8bcf49b - (HEAD, origin/master, origin/HEAD, master) Merge branch 'Issue11647-ClangPatches' (88 seconds ago) <Chris Green>
|\
| * commit:41a4b28 - Apply 0005-Apply-__clang__-blocks-consistent-with-GCC-Intel.patch from issue #11647. (21 minutes ago) <Chris Green>
| * commit:6e9dfd4 - Apply 0004-cpu_timer_test-explicitly-include-cmath.patch from issue #11647. (21 minutes ago) <Chris Green>
| * commit:947b36b - Alternative to 0003-Fully-qualify-cet-namespace-for-cbegin-cend.patch. (21 minutes ago) <Chris Green>
| * commit:3465012 - Apply 0002-Don-t-use-temp-regex-in-sregex_token_iterator.patch from issue #11647. (21 hours ago) <Chris Green>
| * commit:a934cdc - Apply 0001-Use-std-array-in-place-of-boost-array.patch from issue #11647. (21 hours ago) <Chris Green>
|/
*

Changes in art to accommodate: art:e0d2527.

#5 Updated by Christopher Green over 4 years ago

  • Status changed from Accepted to Resolved

#6 Updated by Christopher Green over 4 years ago

Patch 0001-Use-std-array-in-place-of-boost-array.patch was reverted with 3ab6fe4 due to the fact that the array is made persistent, and even ROOT6 cannot currently deal with std::array, quite apart from the likely schema evolution consequences.

#7 Updated by Kyle Knoepfel about 4 years ago

  • Target version changed from 2.01.00 to 2.00.01

#8 Updated by Kyle Knoepfel about 4 years ago

  • Status changed from Resolved to Closed


Also available in: Atom PDF