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

Stores wxObject pointer in a void* instead of a long. #563

Merged
merged 2 commits into from Dec 2, 2019

Conversation

@Randalphwa
Copy link
Contributor

Randalphwa commented Nov 26, 2019

Fixes # 562

The only significant issue preventing building wxFormBuilder and all plugins as a 64-bit target is issue #562 which refers to storing a pointer into a long.

On my fork I am now able to build and run a 64-bit version of wxFormBuilder and all plugin dlls. It does require an additional change to ImportComponentLibrary to load the correct version of the dll, however that's going to be dependent on build script changes, so I'm not including that change in this PR. In addition, just because it builds and runs, doesn't mean it's stable, so I want to spend a lot more time working with the 64-bit version before suggesting adding it as an optional build platform.

Note that I did not wrap the changes in bit-depth conditionals. While the change isn't strictly necessary for 32-bit builds, I don't think it will have any impact on performance. Not using conditionals in this case makes for easier to read code, and of course allows anyone else to build a 64-bit version.

Fixes # 562
@Randalphwa Randalphwa changed the title Stores wxObjeject pointer in a void* instead of a long. Stores wxObject pointer in a void* instead of a long. Nov 26, 2019
@sodevel

This comment has been minimized.

Copy link
Member

sodevel commented Nov 27, 2019

I am going to answer here and not in the Issue to focus the discussion in one place.

That is an interesting discovery you made there however i don't agree with your solution. Multiple wxWidgets elements offer the possibility of storing arbitrary user data but unfortunately they don't use the same approach everywhere. I think instead of fixing the issue on our side by implementing another level of indirection it would be the better option to fix the issue in wxWidgets itself. The wxDataViewListStore uses a wxUIntPtr to store the user data, i think it would be a viable solution in this case as well (or the wxIntPtr to keep it signed like it is now).

@Randalphwa

This comment has been minimized.

Copy link
Contributor Author

Randalphwa commented Nov 28, 2019

I certainly agree that the ideal approach is for the underlying library to change it's code to use platform-independent storage size for arbitrary data. However, that would mean it would be impossible to build a 64-bit version of wxFormBuilder until/if that library was changed, and it would further require building only against that library (or a later version).

Another way of handling this would be to wrap the changes in 64-bit conditionals -- it does, after all, only affect 64-bit builds. Then later on if the wxAui library gets updated, the conditionals could be changed to add that particular library version (i.e., don't use our indirection if building against a library where the UserData we need has been changed to be platform independent).

The reason I didn't suggest that in the first place is it makes the code a bit harder to read. And of course since currently I seem to be the only one desiring a 64-bit build, I could simply leave the change in my fork for possible future inclusion should the request for a 64-bit version become more common.

@sodevel

This comment has been minimized.

Copy link
Member

sodevel commented Nov 28, 2019

I agree that #ifdefs should be used sparingly and also see the issue regarding the library update. Personally i only build on Windows and use some recent master snapshot but this is currently not an option for the other platforms, there we have to use what the distribution offers.

I just saw this change is affecting only one component of one plugin, i think this doesn't add too much complexity and can be added. I have only a few remarks in the following review.

@@ -109,6 +120,9 @@ class AuiToolBar : public wxAuiToolBar
void OnTool( wxCommandEvent& event );

DECLARE_EVENT_TABLE()

private:
std::unordered_map<int, void*> m_aObjects;

This comment has been minimized.

Copy link
@sodevel

sodevel Nov 28, 2019

Member

This element is used only locally and seems to store only wxObject* elements, i would prefer to use this as data type so the casts can be avoided

m_manager->SelectObject( wxobject );
wxObject* wxobject = (wxObject*) GetPtrData(item->GetUserData());

if (NULL != wxobject)

This comment has been minimized.

Copy link
@sodevel

sodevel Nov 28, 2019

Member

Modern-C++: Don't use NULL for pointer values and don't compare pointers explicit to nullptr, a simple implicit test if (wxobject) is preferred

{
m_manager->SelectObject( wxobject );
wxObject* wxobject = (wxObject*) GetPtrData(item->GetUserData());

This comment has been minimized.

Copy link
@sodevel

sodevel Nov 28, 2019

Member

Modern-C++: Please no c-style casts. I'm quite a fan of auto, especially if the type is already used explicit on the right side, so this could be changed to auto* wxobject = static_cast<wxObject*>(GetPtrData(item->GetUserData()));. However this isn't necessary anymore if the lookup map returns wxObject*.

@@ -1261,7 +1275,7 @@ void AuiToolBar::OnDropDownMenu( wxAuiToolBarEvent& event )

if ( item && item->HasDropDown() )
{
wxObject* wxobject = (wxObject*) item->GetUserData();
wxObject* wxobject = (wxObject*) GetPtrData(item->GetUserData());

This comment has been minimized.

Copy link
@sodevel

sodevel Nov 28, 2019

Member

Modern-C++, see below

wxObject* wxobject = (wxObject*) tb->FindTool( event.GetId() )->GetUserData();

if ( NULL != wxobject )
wxAuiToolBarItem* item = tb->FindTool(event.GetId());

This comment has been minimized.

Copy link
@sodevel

sodevel Nov 28, 2019

Member

Modern-C++, see below

@Randalphwa

This comment has been minimized.

Copy link
Contributor Author

Randalphwa commented Nov 29, 2019

I've been trying to match the coding practice that I'm seeing in other areas of the code, even if it's significantly different then how I would normally write the code. So for example, while I would never write if (NULL == ptr) I've seen that being used in other spots, so I did the same to match what I thought was common coding practice for this project. I realize a lot of people have worked on the code and both formatting style and coding methods vary quite a bit. My question is, is there a code module that you would consider to be a fairly good example of the type of coding practices/formatting you would like to see that I could refer to?

The .clang-format for the project only has a few settings. I tried running a few files through it and clang made massive changes, so it clearly can not be used file-wide. I took my own .clang-format, replaced the options that were different and use that to format any code that I work on, but I'm wonder if there is also a source file that contains the formatting you wish all the code was formatted as? If so, I can then use that as a template to make any changes (if needed) to my fork version of .clang-format to cover only any new/changed code that I write?

Finally, the first PR check failed with the error "clang: resource not found". Since I was only checking in a source file change, I assume that's a scripting issue on the server end -- but is there anything I can do on my end to prevent that PR check error?

@sodevel

This comment has been minimized.

Copy link
Member

sodevel commented Nov 29, 2019

The .clang-format together with the policy that every new line of code should follow the style imposed by it as good as possible without touching any surrounding code was introduced by the now kind of fully retired maintainer. I did disagree with certain style elements (especially tab's and braces on next line) but followed the style. Now that i am kind of the sole maintainer i wanted to get some order into the formatting and style so it is easier to use automatic formatting but it turns out this is not so easy.

I applied my formatting changes to the xml files and code templates i did years ago and maintained in our own private fork recently (although im not sure if i should regret not having replaced the tab's in the templates with spaces). I tried every setting of clang-format to format the source but couldn't get a satisfactory result. Biggest problem i have with clang-format is how it splits long lines into multiple lines, i don't want the parameters on column 70 to drop down one by one over 10 lines, i want them to move on the next line and get indented by one level. And then the closing parenthesis on the next line to close that block. Since two years somone is working on a patch to get that closing parenthesis on the next line, hopefully that will be finished soon so i can give it another try.

Best advice i can give on formatting is use common sense and try to blend into the surrounding code. One thing you should not use anymore is that whitespace padding of parentheses. Hopefully somewhere in the near future i find a solution how to deal with current code base and put some auto formatting configuration in place so that there is a more formal definition available and that most importantly can be applied automatically. As a hint what to do today i can only say take a look at the recent commits.

About coding style, Modern C++ is welcome, but because compiler support for C++17 is quite low currently try not to use much of that, i think it is currently only enabled to support some newer attributes.

The CI was also realized by the former maintainer and im not very familiar with it. It seems that Drone is the only one that cannot be configured through the repository, it broke only recently and i hope it is an error on their side and fixes itself like it did with Travis some time ago. If not i think i will simply disable it, i think it was only used to run some clang analyzers over the code anyway. You can just ignore this error for now.

@Randalphwa

This comment has been minimized.

Copy link
Contributor Author

Randalphwa commented Nov 30, 2019

I changed the function names to GetObject and SetObject since they are now specifically storing/retrieving a wxObject* pointer.

In this particular case, I'm reluctant to use auto because neither the variable name or the function name indicates that it is a pointer being returned, not a reference or copy. I realize that currently the variable is not being dereferenced, but I would expect someone reading that code to do something like wxobject.IsKindOf(...) which of course fails because it should be wxobject->IsKindOf(...). It probably comes from 30+ years of using Hungarian notation -- it just looks wrong to use -> on a variable that doesn't start with p... :-)

@Randalphwa

This comment has been minimized.

Copy link
Contributor Author

Randalphwa commented Nov 30, 2019

AppVeyor is now failing with the complaint that InnoSetup is not installed. Hopefully like the drone check this will get fixed on the server side.

@sodevel

This comment has been minimized.

Copy link
Member

sodevel commented Dec 2, 2019

For pointers we are using an explicit auto* for better readability, of the hungarian notation we only use the visibility part for globals/statics/members. Although i do prefer auto, especially for storing results of method calls, to make refactoring easier it is currently not a requirement to use it.

Changes are looking good, thanks for your work.

@sodevel sodevel merged commit 964f29c into wxFormBuilder:master Dec 2, 2019
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/drone/pr Build encountered an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@KeyWorksRW KeyWorksRW deleted the Randalphwa:ptr_fix branch Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.