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

lvdocview: remove (unused) crskin and wolutil related code #542

Merged
merged 2 commits into from
Oct 26, 2023

Conversation

benoit-pierre
Copy link
Contributor

@benoit-pierre benoit-pierre commented Oct 24, 2023

Will allow koreader/koreader-base#1675.


This change is Reviewable

@poire-z
Copy link
Contributor

poire-z commented Oct 24, 2023

I know there is a lot of stuff we don't use in crengine - but let's keep it so 1) we're not the ones breaking the link with other forks and their ability to pick our stuff and 2) it has happened that looking at that unused stuff gives some explanations of why some stuff/arguments/logic in remote stuff we do use is as it is - so it's best if all the context is kept.
So, better wrap if under #if USE_THAT_THING (or #if 0 at worst).

@benoit-pierre
Copy link
Contributor Author

With the amount of custom changes, I think it makes more sense to not be shy about simplifying stuff. Reduce the barrier to entry, reduce the complexity, reduce compilation time, reduce the final library size, reduce the amount of code you have to wade through…

If you want to keep stuff for archeological spelunking, make a branch before cleaning up (I would in fact go as far as removing those unused files too, again less to stuff to wade through).

@poire-z
Copy link
Contributor

poire-z commented Oct 24, 2023

Reduce the barrier to entry, reduce the complexity, reduce compilation time, reduce the final library size, reduce the amount of code you have to wade through…

You're not losing all that with #if'ing (and #if 0 if you really have to, so they look like comments in vim). And I keep my context.
I don't care about the barrier to entry.
I've been doing archeological work for the last 7 years, and there's no doubt this will be needed again, so don't make a barrier to continue :)

@benoit-pierre
Copy link
Contributor Author

Grep does not care about #if 0, but ok…

@poire-z
Copy link
Contributor

poire-z commented Oct 24, 2023

Grep does not care about #if 0

Which is perfectly welcome for my archeological use case.
(And even yours: if you want to change a thing, good to find out why it was made that way, and get confirmations with the possible workarounds in these places that there's indeed something tricky.)

but ok…

Thanks!

@Frenzie
Copy link
Member

Frenzie commented Oct 25, 2023

Grep does not care about #if 0

I believe that was the very point! 😆

Copy link
Contributor

@poire-z poire-z left a comment

Choose a reason for hiding this comment

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

I don't mind removing the other stuff, just keep the long LVDocView::exportWolFile() implementation wrapped in #if 0.
(I remember I once did find in there some explanation for some stuff present elsewhere, may be some TOC or drawBuf stuff - so it will be good to still be able to find some methods possibly only used in there - and now unused. That you could #if 0 if you really want to then :)

@poire-z poire-z merged commit 156d095 into koreader:master Oct 26, 2023
1 check passed
@benoit-pierre benoit-pierre deleted the pr/no_unused_code_compilation branch October 26, 2023 22:30
Frenzie pushed a commit to koreader/koreader-base that referenced this pull request Oct 31, 2023
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.

3 participants