Open PetherPettersson opened 7 years ago
gunnarx
commented
7 years ago ...or rename to remove the collision? Order of header inclusion is not good to rely upon.
It's hard to see how a member name inside a struct can collide, unless this is the Qt special interpretation of the keyword "signals"?
PetherPettersson
commented
7 years ago Yes, renaming is a solution, but i have not checked the extent on which this is used in your code to see if this would be feasible. But good to do if you plan on having vsi available for qt directly.
Qt defines signals as a macro by which you are able to define signals in class definitions, it's an integral part of the qt framework. I am not too sure of what that functionality is called in c++ language. So it's not just a member name.
gunnarx
commented
7 years ago Qt defines signals as a macro by which you are able to define signals in class definitions
= what I meant by "the Qt special interpretation of the word signals"
...which is used by writing
signals:
in a Class definition.
I'm surprised that the Qt MOC does not accurately notice and ignore this totally different usage of the word? Does Qt formally forbid any other usage of the word signals, or is it undefined when this will collide and not?
I am not too sure of what that functionality is called in c++ language
A hack? :)
(by the way signals-and-slots as a concept is supported in some standard C++ libraries nowadays)
Yes, renaming is a solution, but i have not checked the extent on which this is used in your code to see if this would be feasible. But good to do if you plan on having vsi available for qt directly.
I'm wondering how you are using the library and if it's really the expected way. Is your usage of the library code being compiled as a Qt program, thus interpreting the signals word incorrectly? Shouldn't VSI be compiled separately (with a C compiler) and brought into an application as a dynamically linked library? If so, I assume only the headers of the "api" directory need to be visible and used by the application program. Somehow this particular member of the shared memory struct does not seem like it would ever need to be exposed to users IMHO. @dmiespdx should be able to clarify.
dmiespdx
commented
7 years ago I'm inclined to agree with gunnarx that it's behaving as though "signals" is a keyword to Qt. I'll check with our resident Qt expert when he gets in to get his opinion. I think that he's already built VSI in the Qt environment so I'll let you know what he had to do to get around this if that's the case.
I'm currently working on a fairly extensive rewrite of the code that already fixes several of Pether's complaints and I'll make sure I check all of his issues before checking the new code in. The code I'm working on is part of an integration that I'm working on that includes the Vehicle Signal Specification (VSS) module so I'm dealing with many of the issues that Pether ran into myself.
There is an existing issue in the b-tree code module that I need to fix... It was originally written as a normal single process library that used a callback function in the user space to do record comparisons thereby allowing any sort of comparison and key structure the user wishes. When I converted it to use shared memory I only needed to compare arrays of longs so that's what I implemented as an expedient implementation (it did what was needed at the time). However, that doesn't work for strings so I've got an enhancement to it to allow the user to define the type and format of the key elements to allow most common cases like strings, arrays of char, short, int, long, etc. I will need that with the VSI/VSS integration that I'm currently working on as well. I've already made the VSS import a separate call from the VSI core code.
Do either of you have a preference for how to specify the file to be imported? Passing it as an input parameter was mentioned but that's awkward if VSI is used as a library (as was originally intended). Some sort of configuration file is the other obvious solution but so far, this file name is the only thing in the configuration file. An environment variable is also a possible solution. Your suggestions are always welcome.
dmiespdx
commented
7 years ago I just spoke with our Qt expert and he verified that the word "signals" is a Qt keyword used by the MOC compiler. The VSI is supposed to be a plain old C library so I can't even just throw a namespace around it to fix this. I just checked the code and found only a few places where I use "signals" as an exportable name so I'll just change it in the code for safety.
gunnarx is right though when he points out that you should not have this problem unless you are trying to compile the VSI code into your application. It was designed to be used as a shared library and performing a "make" at the top level produces a "libvsi-api.so" file which is what your program should be linking to.
gunnarx
commented
7 years ago I'm inclined to agree with gunnarx that it's behaving as though "signals" is a keyword to Qt.
There are no ifs or buts about that. Both me and Pether confirmed it clearly I thought... But yeah, it seems up to @PetherPettersson to explain now why this happens, since it is a library.
dmiespdx
commented
7 years ago FWIW, the rewrite that I'm currently working on has already eliminated the "signals" variable that had been defined so this will no longer be an issue when I check in the new code.
Our Qt expert also became curious about how many other "keywords" were defined by Qt and he found that there are only 2: "signals" and "slots".
PetherPettersson
commented
7 years ago @gunnarx & @dmiespdx
Hi, thank you for your responses.
gunnarx is right though when he points out that you should not have this problem unless you are trying to compile the VSI code into your application. It was designed to be used as a shared library and performing a "make" at the top level produces a "libvsi-api.so" file which is what your program should be linking to.
I understand, but however we try to link it in the compiler for some reason demands that we also include the core header files as the api headers includes them. We use your make scripts and link with the lib folder and such.
@dmiespdx
What kind of rework have you planned? Does it entail any large changes on how vsi works? We are having internal discussions about vsi here as it doesn't fill our needs completely, but this would be a discussion over email i believe.
As the main issue will be resolved in a later version this could be closed i guess.
dmiespdx
commented
7 years ago Pether,
As you've found out, the VSI code is not really ready for production in it's current form. I'm currently working to integrate VSI, VSS (Vehicle Signal Specification) and the rvi_lib modules. Doing that has exposed many of the issues that you've also run into as well as others that I'm fixing. The charter for the VSI module originally was to be FAST above all so much of the code is geared for that.
Some of the changes I'm making right now are:
If there are things you'd like to see in the code, I'd be happy to consider them.
We can take the discussion offline. My work email is below.
Thanks for the interest in VSI...
Don
On Tue, Apr 4, 2017 at 4:57 AM, Pether Pettersson notifications@github.com wrote:
@gunnarx https://github.com/gunnarx & @dmiespdx https://github.com/dmiespdx
Hi, thank you for your responses.
gunnarx is right though when he points out that you should not have this problem unless you are trying to compile the VSI code into your application. It was designed to be used as a shared library and performing a "make" at the top level produces a "libvsi-api.so" file which is what your program should be linking to.
I understand, but however we try to link it in the compiler for some reason demands that we also include the core header files as the api headers includes them. We use your make scripts and link with the lib folder and such.
@dmiespdx https://github.com/dmiespdx
What kind of rework have you planned? Does it entail any large changes on how vsi works? We are having internal discussions about vsi here as it doesn't fill our needs completely, but this would be a discussion over email i believe.
As the main issue will be resolved in a later version this could be closed i guess.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GENIVI/vehicle_signal_interface/issues/16#issuecomment-291478054, or mute the thread https://github.com/notifications/unsubscribe-auth/AQC0me0Sp-A4AIHWx74ryDNXxsKHxsutks5rsjBFgaJpZM4MxcuJ .
--
Don Mies Systems Architect - Open Source Projects Open Software Technology Center
Email: dmies@jaguarlandrover.com mfeuer@jaguarlandrover.com
M: +1 601-953-3397
Jaguar Land Rover North America, LLC 1419 NW 14th Ave, Portland, OR 97209 Jaguar USA http://www.JaguarUSA.com/index.html | Land Rover USA http://www.LandRoverUSA.com/us/en/lr
Business Details: Jaguar Land Rover Limited Registered Office: Abbey Road, Whitley, Coventry CV3 4LF Registered in England No: 1672070
This e-mail and any attachments contain confidential information for a specific individual and purpose. The information is private and privileged and intended solely for the use of the individual to whom it is addressed. If you are not the intended recipient, please e-mail us immediately. We apologise for any inconvenience caused but you are hereby notified that any disclosure, copying or distribution or the taking of any action in reliance on the information contained herein is strictly prohibited.
This e-mail does not constitute an order for goods or services unless accompanied by an official purchase order.
When trying to use vsi withing a qt application the compiler is crying about a collision of definitions between vsi and qt. This is solved by reordering the import of .h files, and is caused by the _btreet signals on row 402 in sharedMemory.h.
Could be a point to add in documentation.