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

Fixed several issues in Wt::Dbo::collection<> #177

Merged
merged 4 commits into from
Jan 26, 2021

Conversation

skoehler47
Copy link
Contributor

This pull request covers/fixes several issues with Wt::Dbo::collection<>:

  • The collection::iterator operator == may cause an access violation due to a missing null-check in the third case; this can occur if impl_ == nullptr && other.impl_ != nullptr && other.impl_.ended_ == false, which is not covered by the first 2 cases.
  • collection::iterator and collection::const_iterator are derived from std::iterator, which is deprecated in C++17. Since deriving from std::iterator does nothing but adding some type definitions, adding those typedefs directly to iterator/const_iterator solves the deprecation issue while being fully compatible with C++14 and before.
  • const_iterator declares the assignment operator operator = (iterator), but the definition is missing. I added one which mimics the behavior of the const_iterator(iterator) constructor.
  • There were some 32/64bit signed/unsigned type conversion issues in collection::size() and count() functions. I added some static_casts to resolve the numerous warnings caused by this, though i suggest a more thorough fix for the issue

Copy link
Collaborator

@emweb emweb left a comment

Choose a reason for hiding this comment

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

Could you also reword your commits so that they have a short title and a longer description (with line breaks), instead of having the entire commit message on a single long line?

src/Wt/Dbo/collection.h Show resolved Hide resolved
src/Wt/Dbo/collection.h Outdated Show resolved Hide resolved
@skoehler47
Copy link
Contributor Author

Done. I hope it is more to your liking now.

I usually prefer 'using' over 'typedef' too, but decided to go with typedef in this case to keep consistency with the rest of Wt::Dbo.

@emweb
Copy link
Collaborator

emweb commented Jan 26, 2021

Yeah, I suppose the rest of Dbo is still in the old style, but I think for new code we can go ahead and move towards the new style.

I'll make some modifications on your branch. There are also some simple tests I want to add.

Regards,
Roel

skoehler47 and others added 4 commits January 26, 2021 18:16
Wt::Dbo::collection<C>::iterator is no longer derived from
std::iterator, which is deprecated in C++17. All typedefs that were
inherited from std::iterator are now defined directly in the iterator
class, thus preserving full backward compatibility with older
standards. Analogous for Wt::Dbo::collection<C>::const_iterator.
The iterator operator == causes an access violation due to a
missing null-check in the last case; it occurs if impl_ == nullptr,
other.impl_ != nullptr and other.impl_.ended_ == false, which is not
covered by the first 2 cases.

Modification by Roel: Added tests for operator==. They crash without the patch.

Co-authored-by: Roel Standaert <roel@emweb.be>
Added the missing definition for Wt::Dbo::collection<C>::const_iterator
operator= (const iterator&), that was declared, but not defined. The
added implementation mimics the constructors behaviour.
@emweb emweb merged commit b592bf5 into emweb:master Jan 26, 2021
@skoehler47 skoehler47 deleted the fix-dbo-01 branch October 4, 2024 11:03
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