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

Build fixes for clang 8.0 and GCC 9.1 #230

Merged
merged 7 commits into from Jun 27, 2019
Merged

Build fixes for clang 8.0 and GCC 9.1 #230

merged 7 commits into from Jun 27, 2019

Conversation

niklas88
Copy link
Member

@niklas88 niklas88 commented Apr 8, 2019

This is currently just a place to collect changes needed to build with clang 8.0 (which doesn't work yet).

@niklas88
Copy link
Member Author

niklas88 commented Apr 12, 2019

@joka921 so I felt like investigating this a bit and I think clang is right in complaining about the use of _isCompressed in the std::enable_if used to deactivate methods in Vocabulary. This seems to be similar to this SO question. In that the template<typename = std::enable_if…> does not depend on any templating of the method itself and is thus missing the deduction context.

I think I've successfully corrected this for the first occurrence of the problem in 7ff2ef3 but this is quite ugly. What Do you think? We do use both Vocabulary with and without CompressedString right, otherwise we could just drop the uncompressed variant.

This is the error (for the next unfixed occurrence)

/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/8.2.1/../../../../include/c++/8.2.1/type_traits:2331:44: error: no type named 'type' in
      'std::enable_if<false, void>'; 'enable_if' cannot be used to disable this declaration
    using enable_if_t = typename enable_if<_Cond, _Tp>::type;
                                           ^~~~~
/home/schnelle/projects/QLever/src/index/./Vocabulary.h:125:29: note: in instantiation of template type alias 'enable_if_t' requested here
  template <typename = std::enable_if_t<!_isCompressed>>
                            ^
/home/schnelle/projects/QLever/src/index/./Index.h:453:32: note: in instantiation of template class 'Vocabulary<CompressedString>' requested here
  Vocabulary<CompressedString> _vocab;

@niklas88 niklas88 added the cleanup Non functional change for better maintainability or portability label Apr 26, 2019
@niklas88
Copy link
Member Author

GCC 9.1 just landed in Arch Linux and QLever now fails to compile on that as well. This seems to be the same issue as on clang. @floriankramer I assume as a follow Arch user you're seeing this as well? @joka921 pining you because you're most familiar with this part of the code.

/usr/include/c++/9.1.0/type_traits: In substitution of ‘template<bool _Cond, class _Tp> using enable_if_t = typename std::enable_if::type [with bool _Cond = (! Vocabulary<CompressedString>::_isCompressed); _Tp = void]’:
/home/schnelle/projects/QLever/src/index/././Vocabulary.h:270:8:   required from ‘class Vocabulary<CompressedString>’
/home/schnelle/projects/QLever/src/index/./Index.h:453:32:   required from here
/usr/include/c++/9.1.0/type_traits:2426:11: error: no type named ‘type’ in ‘struct std::enable_if<false, void>’
 2426 |     using enable_if_t = typename enable_if<_Cond, _Tp>::type;
….

@niklas88 niklas88 changed the title Fixes from building with clang 8.0 [WiP] Build fixes for clang 8.0 and GCC 9.1 Jun 25, 2019
@floriankramer
Copy link
Member

@niklas88 I'm seeing the error as well running under gcc 9.1. Based upon some reading about SFINAE I did the technique is only applicable to templated functions, as the compiler will otherwise be able to instantiate the function, leading to an error (even if it is never called). Given the requirement for templating I think that your suggested solution of adding an unecessary tempalte parameter that prevents the compiler from instantiating the method might be the best solution, as the only alternative seems to be spezializing the themplate, which includes a lot of code duplication.
@joka921 The version that uses enable_if for the return type does not compile for me using gcc 9.1 when only the function call is commented out. The compiler still instantiates the method, as it only depends on the template parameter of the struct.

@niklas88
Copy link
Member Author

@floriankramer ok have you started changing the functions like in my example correction? Otherwise I would pick up this PR and add them here then we also get the other cleanups.

@floriankramer
Copy link
Member

floriankramer commented Jun 27, 2019

@niklas88 I've created a pr from my local fixes: #261

@niklas88
Copy link
Member Author

niklas88 commented Jun 27, 2019

@floriankramer I've integrated your SFINAE fix here and only needed one additional change to make things build and run with clang 8.0. I'm still seeing a lot of new warnings. I'll see how much work the GCC warnings are and then we can leave the clang warnings for another PR

@niklas88 niklas88 changed the title [WiP] Build fixes for clang 8.0 and GCC 9.1 Build fixes for clang 8.0 and GCC 9.1 Jun 27, 2019
Copy link
Member

@floriankramer floriankramer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@niklas88
Copy link
Member Author

@floriankramer I pushed another commit and now I can't see any GCC 9.1 warnings in our code (still some in clang but that's for later). Since this last commit is trivial I'll merge when Travis gives its okay.

@niklas88 niklas88 merged commit 49c00d5 into ad-freiburg:master Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Non functional change for better maintainability or portability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants