Fix warnings #527

bbshelper · 2023-09-25T13:56:25Z

These are some warnings exposed by enabling -Wall, and are relatively easy to fix. Others like comparison between signed and unsigned integers are much harder to solve. I suggest that we enable -Wall -Werror ASAP to prevent new warnings, while adding waivers for legacy code.

This change is

These warnings can be seen by enabling -Wall.

Frenzie · 2023-09-25T14:04:44Z

crengine/src/lvdocview.cpp

@@ -136,8 +136,6 @@ static css_font_family_t DEFAULT_FONT_FAMILY = css_ff_sans_serif;
 /// minimum EM width of page (prevents show two pages for windows that not enougn wide)
 #define MIN_EM_PER_PAGE     20

-static int def_font_sizes[] = { 18, 20, 22, 24, 29, 33, 39, 44 };


This is used like 10 lines down ftr.

Though strictly I suppose you might argue it should also have USE_LIMITED_FONT_SIZES_SET around it.

You're right. Fixed. I also reverted the change in lvrend.cpp in case of side effects.

NiLuJe

Reviewed 8 of 16 files at r1, 1 of 8 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @bbshelper and @Frenzie)

crengine/src/lvstsheet.cpp line 25 at r3 (raw file):

//#define DUMP_CSS_PARSING

// Helper to debug string parsing, showing current position and context

I imagine that one may have been left in on purpose (but perhaps should be behind ifdefs). Or not. @poire-z will tell us ;).

poire-z · 2023-09-25T20:42:32Z

Will look at all that better by end of week.

But what's with the random reordering of fields ?
I guess it's by field length in bytes, for packing/padding reasons.
I did think about packing for classes/structs used often/a lot - but for most others that are not used often or a lot, I did not. And the current ordering is by field topic, logic, or date added.
These random reordering kills this possible logic - and add not really needed diffs :/

bbshelper · 2023-09-26T00:56:33Z

But what's with the random reordering of fields ?

It's about initialization order: https://stackoverflow.com/questions/1828037/whats-the-point-of-g-wreorder

Honestly I haven't noticed any real problems uncovered in our code by this warning, but eliminating them is desirable.

poire-z · 2023-09-28T23:17:29Z

I also reverted the change in lvrend.cpp in case of side effects.

If you mean that you finally decided to not remove lUInt16 childElementId = child->getNodeId();, it seems like I forgot to remove it as part of 569e4f7, and I think you could remove it. (Or do you see some real possible side effects?)

poire-z · 2023-09-28T23:27:15Z

And the current ordering is by field topic, logic, or date added.
These random reordering kills this possible logic - and add not really needed diffs :/

It's about initialization order

Oh, ok - you did not change my declarations ordering (what I will look at), you just made the initializations (that I will rarely look at) order similar to the declaration order - which is fine and add consistency :) So, fine with me.

poire-z · 2023-09-28T23:34:45Z

crengine/src/lvtinydom.cpp

-    int new_h = new_rc.bottom - base_y;
    int next_fragments_shift_y = new_rc.bottom - orig_rc.bottom;
    // printf("fixing fragment %d (%d ~ %d => %d , +%d => +%d)\n", node->getNodeIndex(), base_y, orig_rc.top, new_rc.top, orig_h, new_h);


Please live line 5440 in, just comment it out (new_h is used in the commented out printf on line 5442 - so if removed, that printf won't work if I need debugging - so best to have them both commented out).

poire-z · 2023-09-28T23:36:23Z

crengine/src/lvstsheet.cpp

-// Helper to debug string parsing, showing current position and context
-static void dbg_str_pos(const char * prefix, const char * str) {
-    printf("/----------|---------- %s\n", prefix);
-    printf("\\%.*s\n", 20, str - 10);
-}
-


Might also be useful in the future, so don't remove it please :) Just wrap it with #if 0 or something like that.

poire-z · 2023-09-28T23:38:53Z

crengine/src/lvstsheet.cpp

-static int substr_compare( const char * sub, const char * & str )
-{


No longer used since a few months ago - but don't kill it, might be useful some day. Just wrap it with /* No longer used or something.

For this and other comments - I will make the change at some later time. It's already midnight here.

Good night :) (no hurry needed)

bbshelper · 2023-09-29T17:17:19Z

I also reverted the change in lvrend.cpp in case of side effects.

If you mean that you finally decided to not remove lUInt16 childElementId = child->getNodeId();, it seems like I forgot to remove it as part of 569e4f7, and I think you could remove it. (Or do you see some real possible side effects?)

I wanted to play safe. After another look at getNodeId(), I believe it can be safely removed. It does have side effects: if the node is swapped to the disk it is loaded back, but it's an implementation detail and doesn't matter to the outside.

- koreader/crengine#527 - koreader/crengine#528 - koreader/crengine#530 - koreader/crengine#531 - koreader/crengine#532 - koreader/crengine#533 - koreader/crengine#534 - koreader/crengine#535 - koreader/crengine#536 - koreader/crengine#538 - koreader/crengine#539 - koreader/crengine#540 - koreader/crengine#541 - koreader/crengine#542 - koreader/crengine#544

fix -Wreorder warnings

e8e0e84

These warnings can be seen by enabling -Wall.

Frenzie reviewed Sep 25, 2023

View reviewed changes

bbshelper force-pushed the fix-warnings branch from a0eb8f7 to b55027c Compare September 25, 2023 14:23

NiLuJe reviewed Sep 25, 2023

View reviewed changes

poire-z reviewed Sep 28, 2023

View reviewed changes

fix warnings about unused functions/variables

09edc77

bbshelper force-pushed the fix-warnings branch from b55027c to 09edc77 Compare October 2, 2023 13:35

poire-z merged commit 3380f70 into koreader:master Oct 3, 2023
1 of 2 checks passed

bbshelper deleted the fix-warnings branch October 3, 2023 14:16

benoit-pierre mentioned this pull request Oct 26, 2023

bump crengine koreader/koreader-base#1681

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix warnings #527

Fix warnings #527

bbshelper commented Sep 25, 2023 •

edited by Frenzie

Loading

Frenzie Sep 25, 2023

Frenzie Sep 25, 2023

bbshelper Sep 25, 2023

NiLuJe left a comment

poire-z commented Sep 25, 2023

bbshelper commented Sep 26, 2023

poire-z commented Sep 28, 2023

poire-z commented Sep 28, 2023

poire-z Sep 28, 2023

poire-z Sep 28, 2023

poire-z Sep 28, 2023

bbshelper Sep 29, 2023

poire-z Sep 29, 2023

bbshelper commented Sep 29, 2023

		static int substr_compare( const char * sub, const char * & str )
		{

Fix warnings #527

Fix warnings #527

Conversation

bbshelper commented Sep 25, 2023 • edited by Frenzie Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiLuJe left a comment

Choose a reason for hiding this comment

poire-z commented Sep 25, 2023

bbshelper commented Sep 26, 2023

poire-z commented Sep 28, 2023

poire-z commented Sep 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbshelper commented Sep 29, 2023

bbshelper commented Sep 25, 2023 •

edited by Frenzie

Loading