Page MenuHomePhabricator

Build errors mitkInteractionConstEvent
Closed, ResolvedPublic

Description

std::strings in class declaration of dynamic module is causing a warning, which leads to an error in the core.

Fix:
changing all const std::strings to const char*.

Event Timeline

New remote branch pushed: bug-15163-FixStringInDynamicModule

Problem only occured in windows, release build with debuginformation enabled.
This is fixed.

Core Changes in
mitkInteractionEventConst.h and
mitkInteractionEventConst.cpp

(string -> const char),

EventConfig, EventFactory, StateMachineTransition and the EventConfigTest,
also needed updating to new data type.

Also the documentation is updated accordingly.

Has this been checked with Sascha? I think std::string would be the much preferable to const char*, because it is a class and less prone to forgetting a delete in the right place..

"const char * const"'s are definitely the right choice here and this is even in the very spirit of C++.

First, there is no need for delete since these strings are statically compiled into the executable.

Second, we do not need/want any of the additional functionality of std::string compared to plain strings.

Third, the declarations/definitions are exactly as safe as the string counterpart in this case.

Fourth, we have unnecessary overhead here since the strings are currently always needed as plain C-Strings, which results in calls to c_str() each and every time these strings are used.

Fifth, the only true alternative is the most ugly one: we would need to temporarily disable a warning for MSVC (which is a correct warning by the way) with conditional pragmas.

There are arguments for both std::string and const char* const.

I actually prefer working with std::string in public API since it is safer (e.g. think about comparisons with inline const char* data). The c_str() calls are unfortunately necessary because mitkPropertyList has a public API using const char* which IMHO is wrong and should be changed to const std::string&. But this is another issue.

The mitkInteractionEventConst class with the std::string members originally was not exported as public API and hence there were no problems with VC10. It has been exported by Christian to be used in unit tests and I am not sure if this was a good idea.

If we really need to expose the XML element/attribute names as part of the public MITK API, we should provide static methods in mitkInteractioniEventConst which return std::string instances containing the data instead of accessing public static members directly.

So as a summary, I would prefer to not expose const char* const members as part of the public MITK API. Either work around the VC10 issue by using static methods instead of members or rethink the unit test and remove the export macro from the mitkInteractionEventConst class again.

New remote branch pushed: bug-15163-ConvertEventConfigMembersToStaticFunctions

Converted members to static methods returning the strings;
we need to expose the xml attributes since they are needed to build custom EventConfig objects (using mitk::PropertyLists)

Implemented changes as discussed with Sascha.
Branch has been tested by Stefan and resolves the problems.

[6106e8]: Merge branch 'bug-15163-ConvertEventConfigMembersToStaticFunctions'

Merged commits:

2013-05-29 14:34:51 Christian Weber [b5365a]
Replaced members by static functions return const std::string


2013-04-29 16:47:24 Sascha Zelzer [4a9833]
MITK 2013.03.02 version update


2013-04-26 17:39:55 Sascha Zelzer [0e56ae]
Merge branch 'bug-14881-fix-opencl-context-creation' into releases/2013-03


2013-04-26 13:33:05 Sascha Zelzer [b11754]
Merge branch 'bug-14915-add-iostream-eventconfig-constructor' into releases/2013-03


2013-04-26 00:15:46 Sascha Zelzer [d26c4e]
Merge remote-tracking branch 'origin/bug-14937-fix-compiler-flag-checks' into releases/2013-03


2013-04-26 00:12:59 Sascha Zelzer [a66c0f]
Merge remote-tracking branch 'origin/bug-14355-fix-resource-compiler-path' into releases/2013-03


2013-04-26 00:10:59 Sascha Zelzer [0910f4]
Merge remote-tracking branch 'origin/bug-14920-fix-git-repo-check' into releases/2013-03


2013-04-26 00:01:10 Sascha Zelzer [b2afbc]
Merge remote-tracking branch 'origin/bug-14841-gpu-volumrendering-memory-leak' into releases/2013-03