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

Switch exprtk to submodule #3965

Merged
merged 2 commits into from
Nov 11, 2017
Merged

Switch exprtk to submodule #3965

merged 2 commits into from
Nov 11, 2017

Conversation

tresf
Copy link
Member

@tresf tresf commented Nov 10, 2017

Moves the Xpressive plugin's exprtk library to a submodule to pull directly from @ArashPartow, removing 38,000+ lines of code from our github repository. :)

For consistency with other plugins, uses TitleCase for the class names (e.g. XpressiveFoo)and renamed folders and classes to match accordingly.

@gnudles, FYI.

@tresf
Copy link
Member Author

tresf commented Nov 11, 2017

Travis-CI looks good. I cancelled Apple after waiting for 20 minutes for it to start. Merging.

@tresf tresf merged commit 231cc82 into LMMS:master Nov 11, 2017
@tresf
Copy link
Member Author

tresf commented Nov 11, 2017

Travis-CI looks good. I cancelled Apple after waiting for 20 minutes for it to start. Merging.

Well, Apple is failing, I guess I merged this too quickly. I narrowed the build failure down to this line:

       inline bool string_to_real(const std::string& s, T& t)
       {
-         const typename numeric::details::number_type<T>::type num_type;
-
          const char_t* begin = s.data();
          const char_t* end   = s.data() + s.size();
-
+         typename numeric::details::number_type<T>::type num_type;
          return string_to_real(begin, end, t, num_type);
       }

A PR with this exact fix was closed here: ArashPartow/exprtk#9

I can reproduce this build error on my machine as well, which is identical to what Travis uses (OS X 10.11.6, Apple LLVM version 8.0.0 (clang-800.0.42.1))

A few people asking similar questions:
https://stackoverflow.com/questions/27743264
https://stackoverflow.com/questions/7411515

This was referenced Nov 11, 2017
@PhysSong
Copy link
Member

PhysSong commented Nov 14, 2017

Build seems to fail on clang < 3.9(I used Clang shipped with Ubuntu) only. I don't know about Apple Clang, but it seems to generate errors only for old Clang.
You may try to compile this:

struct tp {};

int main() {
	const tp t;
	return 0;
}

@tresf
Copy link
Member Author

tresf commented Nov 14, 2017

@PhysSong thanks, that's what I found as well. Unfortunately, this is the default version for both Travis-CI as well as the Mac I use to create our installers from. Should we write a patch?

@tresf tresf mentioned this pull request Nov 14, 2017
@tresf tresf deleted the exprtk branch November 14, 2017 02:24
@ArashPartow
Copy link
Contributor

@tresf I'm planning on fixing this particular 'issue' over the weekend.

The error older clang versions provide turns out to be partially true and false. If the struct has one or more members, the error diagnostic would be correct.

However according to DR-253, the error diagnostic should NOT be made if the struct is empty. The struct in question is used in tag-dispatch and is by design meant to be empty (no members)

That all being said in order to be compatible with both pre/post C++11 versions, the solution can't use the default initialiser "{}", so the only viable option will be to implement a default constructor.

@ArashPartow
Copy link
Contributor

I've updated the library. Let me know if the compilation errors still persist.

@tresf
Copy link
Member Author

tresf commented Nov 27, 2017

@ArashPartow thanks kindly. Testing it out here #4013.

@tresf
Copy link
Member Author

tresf commented Nov 27, 2017

Looks good, merged via 67231cb. Thanks @ArashPartow!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants