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

Clean up some header includes to slightly improve the build time #1319

Merged
merged 11 commits into from
Apr 12, 2024

Conversation

RobinTF
Copy link
Collaborator

@RobinTF RobinTF commented Apr 12, 2024

  • Move the RuntimeParameters (which are only used in a few places but are very expensive to parse) into their own header. They used to be part of the global Constants.h file, which is included almost everywhere.
  • Replace gtest.h by gtest_prod.h in places where only the FRIEND_TEST macro is needed.
  • Move some code in the expression module from header files to .cpp files.
  • Unify the syntax of several includes.

Copy link

codecov bot commented Apr 12, 2024

Codecov Report

Attention: Patch coverage is 88.63636% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 88.37%. Comparing base (e52ad43) to head (69f0090).

Files Patch % Lines
...engine/sparqlExpressions/SparqlExpressionTypes.cpp 72.09% 7 Missing and 5 partials ⚠️
src/engine/sparqlExpressions/SparqlExpression.cpp 90.58% 8 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1319   +/-   ##
=======================================
  Coverage   88.36%   88.37%           
=======================================
  Files         313      316    +3     
  Lines       28444    28440    -4     
  Branches     3138     3138           
=======================================
- Hits        25134    25133    -1     
+ Misses       2173     2168    -5     
- Partials     1137     1139    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@joka921 joka921 left a comment

Choose a reason for hiding this comment

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

I have two small remarks. If you address them,
we can quickly merge this (there are also small conflicts with the master, but they should be easy to fix).

Comment on lines 465 to 471
return a.visit([&](const auto& aValue) {
return b.visit([&](const auto& bValue) -> ComparisonResult {
if constexpr (requires() { std::invoke(comparator, aValue, bValue); }) {
return fromBool(std::invoke(comparator, aValue, bValue));
} else {
AD_FAIL();
}
Copy link
Member

Choose a reason for hiding this comment

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

With our updated compiler versions and consistent with a change that I have in the pipeline we can now simply write

return std::visit([&}(const auto& aValue, const auto& bValue) {...}, a, b);

Copy link
Member

Choose a reason for hiding this comment

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

Okay, we can't because those are not std::variant at all, but I am currently merging the conflict there myself.

Comment on lines 8 to 11
#include "../util/File.h"
#include "../util/Serializer/ByteBufferSerializer.h"
#include "../util/Serializer/FileSerializer.h"
#include "../util/Serializer/SerializeHashMap.h"
Copy link
Member

Choose a reason for hiding this comment

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

These can and should also all be "util/Serializer/..." (absolute paths) while you are at it.

Copy link

Quality Gate Passed Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@joka921 joka921 changed the title Slightly optimize build time Clean up some header includes to slightly improve the build time Apr 12, 2024
@joka921 joka921 merged commit 6130f5d into ad-freiburg:master Apr 12, 2024
19 checks passed
@RobinTF RobinTF deleted the optimize-build-time branch April 12, 2024 13:16
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