You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celix.apache.org by GitBox <gi...@apache.org> on 2020/01/10 14:07:13 UTC
[GitHub] [celix] Oipo opened a new pull request #139: Add parsing of version
and filter on version when calling useService*.
Oipo opened a new pull request #139: Add parsing of version and filter on version when calling useService*.
URL: https://github.com/apache/celix/pull/139
Have not been able to run the test_framework target due to errors when building.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [celix] Oipo edited a comment on issue #139: Add parsing of version
and filter on version when calling useService*.
Posted by GitBox <gi...@apache.org>.
Oipo edited a comment on issue #139: Add parsing of version and filter on version when calling useService*.
URL: https://github.com/apache/celix/pull/139#issuecomment-578503539
@pnoltes This version includes the requested changes. It is ready for review.
Summary of what changed:
* removed zero-initialization of structs due to incorrect g++ 4.x warning
* Refactored creating the LDAP version filter to utils
* Re-enabled version and version_range tests and fixed them
* Added assert to warn for double free
* Fix segfault when comparing versions with NULL qualifiers
* Partial fix for #140
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [celix] pnoltes commented on a change in pull request #139: Add
parsing of version and filter on version when calling useService*.
Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #139: Add parsing of version and filter on version when calling useService*.
URL: https://github.com/apache/celix/pull/139#discussion_r365586437
##########
File path: libs/framework/tst/bundle_context_services_test.cpp
##########
@@ -87,7 +87,7 @@ TEST(CelixBundleContextServicesTests, registerMultipleAndUseServices) {
};
const char *calcName = "calc";
- struct calc svc;
+ struct calc svc{};
Review comment:
This generates an error. Add {nullptr} initialisation to (function pointer) field to prevent this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [celix] Oipo commented on a change in pull request #139: Add
parsing of version and filter on version when calling useService*.
Posted by GitBox <gi...@apache.org>.
Oipo commented on a change in pull request #139: Add parsing of version and filter on version when calling useService*.
URL: https://github.com/apache/celix/pull/139#discussion_r370988766
##########
File path: libs/framework/tst/bundle_context_services_test.cpp
##########
@@ -87,7 +87,7 @@ TEST(CelixBundleContextServicesTests, registerMultipleAndUseServices) {
};
const char *calcName = "calc";
- struct calc svc;
+ struct calc svc{};
Review comment:
So the error is actually an error of the g++ 4.x and 5.x series. The warning is used to indicate class types that are improperly initialized. Since c++11, empty initialization means [zero initialization for non-union class types](https://en.cppreference.com/w/cpp/language/zero_initialization) (as is the case here). As this does not lead to half-initialized Therefore, since version 6.1 ([godbolt](https://godbolt.org/z/T6yuJD)), g++ includes the following exception for this warning:
`in C++ this option does not warn about the empty { } initializer` [[docs]](https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html)
As it is a hassle to not apply this rule for these few instances, because it is test code and every time the calc field is properly filled, I will simply revert the changes here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [celix] pnoltes commented on a change in pull request #139: Add
parsing of version and filter on version when calling useService*.
Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #139: Add parsing of version and filter on version when calling useService*.
URL: https://github.com/apache/celix/pull/139#discussion_r365586926
##########
File path: libs/framework/src/service_tracker.c
##########
@@ -32,6 +32,8 @@
#include "celix_log.h"
#include "bundle_context_private.h"
#include "celix_array_list.h"
+#include "../../utils/src/version_range_private.h"
Review comment:
Do not include with relative paths and do not include "private" headers (headers in the src dirs).
I would prefer that a function for transforming a version ranges to ldap filter is added to the utils library:
typedef struct versionRange version_range_t;
/**
* Returns the LDAP filter for a version range. Caller is owner of the returned string.
* If version range is not valid, NULL will be returned.
*/
char* versionRange_createLDAPFilter(version_range_t *range)
/**
* construct a LDAP filter for the provided version range.
* The string will be created in the provided buffer, if the buffer is big enough.
* Returns true if successful.
*/
bool versionRange_constructLDAPFilter(version_range_t *range, char* buffer, size_t bufferLength);
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [celix] Oipo commented on issue #139: Add parsing of version and
filter on version when calling useService*.
Posted by GitBox <gi...@apache.org>.
Oipo commented on issue #139: Add parsing of version and filter on version when calling useService*.
URL: https://github.com/apache/celix/pull/139#issuecomment-578503539
@pnoltes This version includes the requested changes. It is ready for review.
Summary of what changed:
* removed zero-initialization of structs due to incorrect g++ 4.x warning
* Refactored creating the LDAP version filter to utils
* Re-enabled version and version_range tests and fixed them
* Added assert to warn for double free
* Partial fix for #140
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [celix] Oipo commented on a change in pull request #139: Add
parsing of version and filter on version when calling useService*.
Posted by GitBox <gi...@apache.org>.
Oipo commented on a change in pull request #139: Add parsing of version and filter on version when calling useService*.
URL: https://github.com/apache/celix/pull/139#discussion_r365989053
##########
File path: libs/framework/src/service_tracker.c
##########
@@ -32,6 +32,8 @@
#include "celix_log.h"
#include "bundle_context_private.h"
#include "celix_array_list.h"
+#include "../../utils/src/version_range_private.h"
Review comment:
While I agree that including private headers is nasty, the source code in service_tracker.c needs the full definition of the structs in those headers, otherwise it simply does not compile. Would you be ok with removing the private header files and moving the definitions of the structs into version_range.h?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [celix] Oipo commented on a change in pull request #139: Add
parsing of version and filter on version when calling useService*.
Posted by GitBox <gi...@apache.org>.
Oipo commented on a change in pull request #139: Add parsing of version and filter on version when calling useService*.
URL: https://github.com/apache/celix/pull/139#discussion_r365983008
##########
File path: libs/framework/tst/bundle_context_services_test.cpp
##########
@@ -87,7 +87,7 @@ TEST(CelixBundleContextServicesTests, registerMultipleAndUseServices) {
};
const char *calcName = "calc";
- struct calc svc;
+ struct calc svc{};
Review comment:
Do you mean a compiler error? This online example shows that it properly initializes the struct with 0: http://cpp.sh/8xalo
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [celix] Oipo commented on a change in pull request #139: Add
parsing of version and filter on version when calling useService*.
Posted by GitBox <gi...@apache.org>.
Oipo commented on a change in pull request #139: Add parsing of version and filter on version when calling useService*.
URL: https://github.com/apache/celix/pull/139#discussion_r370988766
##########
File path: libs/framework/tst/bundle_context_services_test.cpp
##########
@@ -87,7 +87,7 @@ TEST(CelixBundleContextServicesTests, registerMultipleAndUseServices) {
};
const char *calcName = "calc";
- struct calc svc;
+ struct calc svc{};
Review comment:
So the error is actually an error of the g++ 4.x and 5.x series. The warning is used to indicate class types that are improperly initialized. Since c++11, empty initialization means [zero initialization for non-union class types](https://en.cppreference.com/w/cpp/language/zero_initialization) (as is the case here). As this does not lead to half-initialized class types, there is no reason to give a warning. Therefore, since version 6.1 ([godbolt](https://godbolt.org/z/T6yuJD)), g++ includes the following exception for this warning:
`in C++ this option does not warn about the empty { } initializer` [[docs]](https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html)
As it is a hassle to not apply this rule for these few instances, because it is test code and every time the calc field is properly filled, I will simply revert the changes here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [celix] pnoltes merged pull request #139: Add parsing of version
and filter on version when calling useService*.
Posted by GitBox <gi...@apache.org>.
pnoltes merged pull request #139: Add parsing of version and filter on version when calling useService*.
URL: https://github.com/apache/celix/pull/139
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services