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

Clang-cl: Fix #739 compatibility macros & add to AppVeyor #741

Closed
wants to merge 86 commits into from

Conversation

Farwaykorse
Copy link
Contributor

@Farwaykorse Farwaykorse commented Oct 15, 2018

As described in #739.
When building on Windows with clang-cl.exe (the compatible replacement for cl.exe), both the macros _MSC_VER and __clang__ are defined.

To get the temporary compatibility fix (#703) working with clang-cl, the check for __clang__ has to come first.


Adding compilation tests with AppVeyor:

  • Investigate curious test failure in Travis CI
    Build 26: on Linux COMPILER=g++-6 BUILD_TYPE=Release GSL_CXX_STANDARD=14
    Resolves [Travis CI] Test broken size fails with GCC 6.5 #745
  • Compiling with the latest clang-cl at AppVeyor
    Using the VS 2017 and the LLVM toolset integration (vs extension available on AppVeyor). This tests the use of clang-cl from VS projects.
  • Temporary fix for the lld-linker and vcpkg (version installed on AppVeyor) --> No issue with Ninja
    (link using link.exe or update the installed vcpkg version)
  • Blocking errors
    • "error : cannot use 'try'/'throw' with exceptions disabled" while building Catch
    • -fno-exceptions is not recognised by clang-cl'; results in: -Wunknown-argument 100f2b9
    • No support for -std=c++1z; ...
      clang-cl.exe : error : unknown argument ignored in clang-cl: '-std=c++1z' [-Werror,-Wunknown-argument]
      Fix: When building with clang-cl use the version of CMake installed on AppVeyor (v3.12.2) . 9a625ab
      Is there a reason for using CMake v3.8.0? Yes: Issue CMake version #487 discusses the use of older CMake versions to ensure accessibility.
  • Potentially valid warnings
    • -Winvalid-noreturn 2238c47 Enabled by Default
      gsl\include\gsl/gsl_assert(123): error : function declared 'noreturn' should not return [-Werror,-Winvalid-noreturn]
    • -Wnewline-eof 137367b -Weverything
      gsl\tests\multi_span_tests.cpp(1785): error : no newline at end of file [-Werror,-Wnewline-eof]
    • -Wunused-member-function 337a03c -Weverything
      gsl\tests\span_tests.cpp(57): error : unused member function 'operator&' [-Werror,-Wunused-member-function]
    • -Wdouble-promotion f810ce7 -Weverything
      gsl\tests\notnull_tests.cpp(196): error : implicit conversion increases floating-point precision: 'float' to 'double' [-Werror,-Wdouble-promotion]
    • -Wmissing-noreturn 1b15a73 -Weverything
      gsl\tests\no_exception_ensure_tests.cpp(29): error : function 'test_terminate' could be declared with attribute 'noreturn' [-Werror,-Wmissing-noreturn]
      gsl\tests\no_exception_throw_tests.cpp(28): error : function 'test_terminate' could be declared with attribute 'noreturn' [-Werror,-Wmissing-noreturn]
    • -Wmissing-variable-declarations 329da83 -Weverything
      gsl\tests\utils_tests.cpp(83): error : no previous extern declaration for non-static variable 'j' [-Werror,-Wmissing-variable-declarations]
  • Update README.md: supported platform Clang-cl 7.0.0 on Windows. 07ec87b

Extra:

Improvements: (After review by annagrin)

  • Separate CMake generation from build. 4631e6e
  • "Clearer"/less convoluted compile flag configuration for the tests. caa29d0
    + 'flatter' decision structure; - some duplication; + more specific to each compiler
    • Remove redundant warning flags for clang-cl (checked documentation since v4.0.0)
      Note: /W4 enables -Wall and -Wextra. 2bdb8f9 767f34c 595a726
    • Remove flag for clang-cl -fno-strict-aliasing no issues building without it. b1e63fd
  • Move suppression of -Wunused-member-function from span_tests.cpp to configuration.
    note: With -Weverything add: $<$<EQUAL:${GSL_CXX_STANDARD},14>:-Wno-unused-member-function> c98c546
  • Why those additional -Wno-* flags? - Local issue. Remove flags. 7b85156 - pushed warning level.
    • Check changes. (marked in this checklist)
  • Different solution for [[noreturn]] in 2238c47
  • Find first working CMake version. Proper flags for clang-cl. v7.0.0 does not recognize -std=c++1z.
    Both CMake and clang on AppVeyor are updated regularly. Unless both can be pinned, similar issues will appear again. Besides, this exercises the usage of the latest CMake release.
  • Return MSBuild generators. 2197612
  • Push warning level.
    • LLVM/clang-cl: -Weverything 9ef9808
    • LLVM/clang++ (Linux & OSX): -Weverything
      • -Wweak-vtables - suppressed fcbbe1f
      • -Wundef on macro _MSC_VER eb60986
      • -Wpadded in gsl_util:51 and multi_span:1231 - suppressed fe882ce
      • -Wundefined-func-template only in clang v5.0.2. e7812cb
      • -Wdeprecated solved e7812cb

        definition of implicit copy assignment operator for 'static_bounds' is deprecated because it has a user-declared copy constructor

  • Clang-cl compilation with VS2015 (using the v140 MS STL). f6f7579
    • -Wkeyword-macro Clang does not require the constexpr disabling macro. f40a69e

Resolves #739

@annagrin
Copy link

annagrin commented Nov 5, 2018

@Farwaykorse thanks for the fix, can we also add test cases in appveyor.yml to make this always true?

@Farwaykorse
Copy link
Contributor Author

Compiling with clang-cl in Appveyor seems possible, but I don't have any experience with this.

@Farwaykorse
Copy link
Contributor Author

I've made an error while reordering the options.
I'll wait for Appveyor to finish the last run, to see if there are more issues, before committing.

Farwaykorse added a commit to Farwaykorse/GSL that referenced this pull request Nov 13, 2018
Temporarily disable msvc compilation for faster testing (microsoft#741)
Temporarily disable msvc compilation for faster testing (microsoft#741)
@Farwaykorse
Copy link
Contributor Author

Farwaykorse commented Nov 13, 2018

Issue with lld-link and the vcpkg install:
Should be fixed when Appveyor updates their vcpkg version. (Requires: >"2018.10.20".)

@Farwaykorse Farwaykorse changed the title Fix #739 correct cppcorecheck warnings for clang-cl Fix #739 correct cppcorecheck warnings for clang-cl [in-progress] Nov 14, 2018
Farwaykorse added a commit to Farwaykorse/GSL that referenced this pull request Nov 14, 2018
Farwaykorse added a commit to Farwaykorse/GSL that referenced this pull request Nov 14, 2018
@Farwaykorse
Copy link
Contributor Author

Farwaykorse commented Dec 4, 2018

Changes:

  • The warning level with LLVM/clang is now pushed to -Weverything.
    • No issues with clang-cl on Windows.
    • Compilation with clang++ on Linux and OSX.
      • -Wdeprecated solved, please review. e7812cb
        GSL/include/gsl/multi_span:598:15: warning: definition of implicit copy assignment operator for 'static_bounds<-1>' is deprecated because it has a user-declared copy constructor [-Wdeprecated]
            constexpr static_bounds(const static_bounds&) noexcept = default;
                      ^
        
      • -Wpadded currently suppressed, might be solvable.
        gsl_util:51:7: warning: padding size of 'gsl::final_action<std::_Bind<void (*(std::reference_wrapper<int>))(int &)> >' with 7 bytes to alignment boundary [-Wpadded]
        multi_span:1231:7: error: padding size of 'gsl::multi_span<const int, 2>' with 7 bytes to alignment boundary [-Werror,-Wpadded]
        
      • -Wweak-vtables currently suppressed, might be solvable.
        gsl_assert:91:8: error: 'fail_fast' has no out-of-line virtual method definitions; its vtable will be emitted in every translation unit [-Werror,-Wweak-vtables]
        gsl_util:97:8: error: 'narrowing_error' has no out-of-line virtual method definitions; its vtable will be emitted in every translation unit [-Werror,-Wweak-vtables]
        
  • The AppVeyor script now supports both MSBuild and Ninja-build.
    The current setup uses MSBuild by default and Ninja for two cases:
    • VS2017: There is a bug in the LLVM integration extension on AppVeyor.
    • VS2015: MSBuild is not supported, since LLVM v7.0.0.
  • Add clang-cl compilation with VS2015 (using the v140 MS STL).

@annagrin
Copy link

@Farwaykorse I will resolve conflicts caused by my changes. Thanks @Farwaykorse!

@annagrin
Copy link

added PR #762 to resolve conflicts

@Farwaykorse
Copy link
Contributor Author

Farwaykorse commented Jan 15, 2019

Thanks @annagrin and @CaseyCarter for your friendly and constructive feedback.

annagrin pushed a commit that referenced this pull request Jan 15, 2019
* Added c++17 test configurations for clang5.0 and clang6.0

* Fix #739 correct cppcorecheck warnings for clang-cl

* Add clang-cl  configurations

* Corrections Appveyor;
Temporarily disable msvc compilation for faster testing (#741)

* Add path to clang-cl.exe (#741)

* Escape backslash in path (#741)

* Update vcpkg (#741)

* Check vcpkg version; try without building vcpkg; use latest clang-cl from path (#741)

* Fix blocks in ps script (#741)

* Try accessing APPVEYOR_BUILD_FOLDER variable (#471)

* Update span size() bug confirmation test for GCC 6.5 (#741)

* MSVC flags to Clang-cl; disable c++98-compat and undefined macro warnings (#741)

* Suppress clang warning on tests (missing-prototypes) (#741)

* Fix clang warning -Wnewline-eof (#741)

* Fix clang warning -Wdouble-promotion (#741)

* Set linker explicitly

* Clean condition statement

* For Clang, fallback to the AppVeyor installed version of CMake

* Fix clang warning -Wmissing-variable-declarations

* Fallback to the MSVC linker until vcpkg has been updated

* Revert "Fallback to the MSVC linker until vcpkg has been updated"

This reverts commit 7263f32.

* Fix clang warning -Wunused-member-function

* Fix clang warning -Wmissing-noreturn

* Fix clang warning -Winvalid-noreturn on Windows

* Add macro block end comment on large #if blocks

* Workaround: fallback to mscv link.exe

* Workaround: get msvc paths into PowerShell through intermediate file

* Workaround: fix, remove "PATH=" from text

* Workaround: try with full-path; and return user PATH

* Workaround: fix, escape backslashes

* Revert all since "Workaround: fallback to mscv link.exe" did not work on AppVeyor

This reverts the commits:
bda3d6a
9706293
0f4fb04
1b0c19a
a5739ea

* Suppress output of git pull; remove vcpkg from cache

* Re-enable AppVeyor builds for all platforms

* Correct typo

Co-Authored-By: Farwaykorse <Farwaykorse@users.noreply.github.com>

* Add Clang-cl 7.0.0 to the supported platforms

* Revert "Fix clang warning -Wunused-member-function"

This reverts commit 6fe1a42.

* Fix or locally suppress clang warning -Wunused-member-function

* format touched code and correct comment

* git pull --quiet

* fix logic error in workaround

* fix missing bracket

* Suppress output of mkdir

* Replace MSBuild with Ninja

* Suppress output of 7z

* Add architecture flags for Clang

* Drop workaround for lld-link

* 7-zip Overwrite and Alternative output suppression without suppressing errors

Replaces 3c1c079

* AppVeyor setup and CMake before build

* reorder compiler configuration

* remove unnecessary

* remove -fno-strict-aliasing

* remove -Wsign-conversion, since before v4.0 part of -Wconversion

* -Wctor-dtor-privacy is GCC only

* remove -Woverloaded-virtual part of -Wmost, part of -Wall

* add -Wmissing-noreturn

* remove the pragmas for -Wunused-member-function

* Re-add MSBuild generator on AppVeyor

* Print CMake commands

* Add MSBuild toolset selection

* Separate Architecture setting

* clang-cl: add -Weverything

* clang-cl -Wno-c++98-compat

* clang-cl -Wno-c++98-compat-pedantic

* clang-cl -Wno-missing-prototypes

* clang-cl C++14 -Wno-unused-member-function

* clang-cl -Wundef __GNUC__

* clang++: add -Weverything

* clang++ -Wno-c++98-compat

* clang++ -Wno-c++98-compat-pedantic

* clang++ -Wno-missing-prototypes

* clang++ -Wno-weak-vtables

* clang++ C++14 -Wno-unused-member-function

* clang++ fix -Wundef _MSC_VER

* clang++ -Wno-padded

* clang++ solve -Wdeprecated

* Add AppleClang compiler target
Since CMake v3.0 use of Clang for both is deprecated

* clang++ v5.0 C++17 -Wno-undefined-func-template

* Add VS2015 + LLVM/clang-cl to AppVeyor

* Do not disable constexpr when compiling with clang-cl on Windows

* Clean-up clang-only warnings (now under -Weverything)

* Revert "Fix clang warning -Winvalid-noreturn on Windows"

This reverts commit 2238c47.

* Suppress -Winvalid-noreturn for the MS STL noexception workaround

* CMake: put preprocessor definition in target_compile_definitions

* Solve compiler warning C4668: __GNUC__ not defined
luncliff added a commit to luncliff/coroutine that referenced this pull request Jan 16, 2019
* Changed GSL to reference master branch
  * microsoft/GSL#741
* TravisCI windows build
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

Successfully merging this pull request may close these issues.

[Travis CI] Test broken size fails with GCC 6.5 Compatibility fix GSL_SUPPRESS not working with clang-cl.exe
3 participants