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

CMake: Fix Python3 and Doxygen invocations. #49

Merged
merged 1 commit into from
Sep 6, 2019

Conversation

tamaskenez
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Sep 4, 2019

Codecov Report

Merging #49 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #49      +/-   ##
=======================================
+ Coverage   96.99%   97%   +<.01%     
=======================================
  Files          13    13              
  Lines         766   768       +2     
=======================================
+ Hits          743   745       +2     
  Misses         23    23
Impacted Files Coverage Δ
include/internal/csv_row.cpp 100% <ø> (ø) ⬆️
include/internal/row_buffer.hpp 100% <ø> (ø) ⬆️
include/internal/csv_reader.cpp 99.6% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c7830f...fdeb7aa. Read the comment docs.

Copy link
Owner

@vincentlaucsb vincentlaucsb left a comment

Choose a reason for hiding this comment

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

Hi @tamaskenez, thanks for submitting a PR. I like a lot of the changes I see here. I'm just wondering if it's not necessary to specify the optimization flags for g++. I'm not the biggest CMake expert.

@tamaskenez
Copy link
Contributor Author

There are a few things I need to say:

  • Sorry about the many changes, I did not want to include all of these changes in to the 2 PRs. I just pushed new commits to my master and forgot that they will show up in the PR. So let's put this on hold, tomorrow I'll remove the unwanted changes and you can review again.
  • In CMake the general rule is to not check CMAKE_BUILD_TYPE ever, because it's meaningless with IDE-generators (like MSVC, Xcode).
  • You can still set config dependent flags with CMAKE_CXX_FLAGS_DEBUG, etc... but CMake already adds all the necessary -O flags to DEBUG and non-DEBUG builds.
  • I probably made one error here: I removed -pthread but the parser does need pthread with gcc, right? Even if it does, there's a better way to add it, I'll do it tomorrow.

@vincentlaucsb
Copy link
Owner

Yes the CSV parser uses threads and won't compile without -pthread

@tamaskenez
Copy link
Contributor Author

I removed the unnecessary changes from my master, so this is only 1 file, please review.

@vincentlaucsb
Copy link
Owner

Do you think it's possible to incorporate the suggestions from #6?

@tamaskenez
Copy link
Contributor Author

Do you think it's possible to incorporate the suggestions from #6?

Yes, I can do it. I mean, I suggest not to extend to scope of this PR. I'll do it in a separate CL.

@vincentlaucsb
Copy link
Owner

Yes you're right. Thanks again for your help!

@vincentlaucsb vincentlaucsb merged commit bdfb5cf into vincentlaucsb:master Sep 6, 2019
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.

2 participants