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

Fix num_alert_types #306

Merged
merged 1 commit into from
Dec 4, 2015
Merged

Fix num_alert_types #306

merged 1 commit into from
Dec 4, 2015

Conversation

aldenml
Copy link
Contributor

@aldenml aldenml commented Dec 3, 2015

Fix num_alert_types.

@aldenml aldenml force-pushed the fix-log-alerts branch 4 times, most recently from a31686c to 43636fa Compare December 3, 2015 20:18
@@ -2496,7 +2537,7 @@ namespace libtorrent
#undef TORRENT_DEFINE_ALERT_PRIO
#undef TORRENT_CLONE

enum { num_alert_types = 89 };
enum { num_alert_types = 90 };
Copy link
Owner

Choose a reason for hiding this comment

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

I get this fix.. what about the other changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other changes are because num_alert_types is not dynamic. It seems to me a little odd not having the right amount of alert types even if TORRENT_DISABLE_LOGGING is defined. Specifically for picker_log_alert there was no constructor if TORRENT_DISABLE_LOGGING and peer_alert has no default constructor.

Copy link
Owner

Choose a reason for hiding this comment

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

ah. the intention of this enum is really "max_alert_index" + 1. It makes it possible to use alert types as indices into a dense array. If some alerts will never actually be posted is not really relevant for that purpose.
are you using it for anything where it's important that it indicates the actual number of alerts that exists?
do you think it should be called something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'm compiling with logging enabled. Before give up with this PR in the current form, I will explain a problem I'm facing (see in the PR's comment).

@aldenml
Copy link
Contributor Author

aldenml commented Dec 4, 2015

rebasing.

@aldenml
Copy link
Contributor Author

aldenml commented Dec 4, 2015

The main motivation of this PR is that I'm dealing with a way to efficiently cast the alerts in a swig/java world. Having the types in a consecutive sequence allows the java compiler apply heavy optimizations in a dense switch/case statement. This optimization can vary with a function of how many cases and how many holes are in the cases.

@aldenml
Copy link
Contributor Author

aldenml commented Dec 4, 2015

I will resubmit this PR with a reduced set of changes.

@aldenml aldenml changed the title Fix no existing alert types if disabled logging for consistency. Fix num_alert_types Dec 4, 2015
arvidn added a commit that referenced this pull request Dec 4, 2015
@arvidn arvidn merged commit 523bc75 into arvidn:master Dec 4, 2015
@aldenml aldenml deleted the fix-log-alerts branch December 4, 2015 13:29
aldenml referenced this pull request in frostwire/frostwire-jlibtorrent Dec 4, 2015
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