fix -Wall warnings
#579
fix -Wall warnings
#579
Conversation
|
Looks like you did not touch anything related to LVFontGlyphCacheItem, so mentionning it - I see this one too often: |
|
Let me do a pass with clang. |
| else if ( vertical_align_flag == LTEXT_VALIGN_MIDDLE ) { | ||
| assert(top_to_baseline != -1); | ||
| assert(baseline_to_bottom != -1); |
There was a problem hiding this comment.
Other branches assign them, or do arithmetic with them. Why only here?
(There's some sane logic, the only outer branch that don't set and use it is when LTEXT_OBJECT_IS_PAD / LTEXT_WORD_IS_PAD, which I admit is a bit strange, checking LTEXT_OBJECT, setting LTEXT_WORD, checking LTEXT_OBJECT...)
There was a problem hiding this comment.
Same as above: that's where the compiler detected a possible problem.
| @@ -8016,6 +8013,7 @@ void ldomNode::initNodeRendMethod() | |||
| if (eoc) | |||
| break; | |||
| } | |||
| assert(elemId != -3); | |||
There was a problem hiding this comment.
OK for -3 (as we use -1 and -2 for other flags).
Anyway, is this just fine putting assert when we can't make out the logic, or should I look at how this could happen and rework a logic around that?
|
For the indentation issues: we compile with |
|
Regarding asserts: they are disabled in release mode, so good to shut up compiler warnings, detect coding/logic mistakes, possible bugs. |
"everywhere" meaning where there are warnings right ? :) (Keep changes minimal to avoid messing git blame). |
Yes, only to fix the warnings, minimal changes. |
``` template-id not allowed for constructor/destructor in C++20 ```
About (possibly) unused functions.
|
And regarding the linter (cppcheck / clang-tidy) warnings: I don't think the way they are currently run is that useful. I've added support locally to the build system, taking advantage of the fact that both linters can use the compilation database generated by cmake, and I'm not getting the same warnings as on GA. See integration here. I'm not sure if it's because locally I make sure the Maybe we should revisit the And those are used in crengine headers, so currently part of the public API.
|
benoit-pierre
force-pushed
the
pr/fix_-Wall_warnings
branch
from
e160c3a
to
43cb034
Compare
|
Anyway, I did another pass with clang, other targets. Except for the aforementioned lvref warnings, the only compiler warnings left are clang grumbling about VLAs. cppcheck is happy (locally). clang-tidy still complains a lot (locally, again). Those warnings are not obvious (and a number of them may be false positives related to lvref, again). |
|
And clang-tidy takes forever… There's some cmake support for running clang-tidy automatically as part of the compilation, but without some sort of cache, that would be painful. |
| @@ -1848,7 +1848,7 @@ class LVFormatter { | |||
| int tabIndex = -1; | |||
| #if (USE_FRIBIDI==1) | |||
| FriBidiLevel lastBidiLevel = 0; | |||
| FriBidiLevel newBidiLevel; | |||
| FriBidiLevel newBidiLevel = 0; | |||
There was a problem hiding this comment.
No "value assigned is never used" warning with that?
| lString32 ldomXRange::getRangeText( lChar32 blockDelimiter, int maxTextLen, lChar32 imageReplacement, LVArray<ldomNode*> * imageNodes ) | ||
| lString32 ldomXRange::getRangeText( lChar32 blockDelimiter, lChar32 imageReplacement, LVArray<ldomNode*> * imageNodes ) |
There was a problem hiding this comment.
Some stuff in cre.cpp provide maxTextLen=8192 (which ok, is not used), so don't forget to update it.
|
No real thought / opinion about #579 (comment) , so go ahead as you please :) |
Re-read that comment ;) The way the linters are run on GA does not match the actual build configuration. I get no such warning when running with koreader/koreader-base@941fbbf locally (Arch Linux, clang-tidy 18.1.8) or in docker (using |
…ings The function actually parses an unsigned integer.
``` warning: Rethrowing current exception with 'throw;', it seems there is no current exception to rethrow. If there is no current exception this calls std::terminate(). ```
Comment out unused & problematic code:
```
crengine/include/lvref.h:395:16: warning: Reference to temporary returned. [returnTempReference]
return LVRef(NULL);
^
crengine/include/lvref.h:396:15: warning: Reference to temporary returned. [returnTempReference]
return LVRef( new T( *_ptr ) );
```
``` warning: Value stored to 'pDiag' is never read [clang-analyzer-deadcode.DeadStores] ```
``` warning: Rethrowing current exception with 'throw;', it seems there is no current exception to rethrow. If there is no current exception this calls std::terminate(). ```
benoit-pierre
force-pushed
the
pr/fix_-Wall_warnings
branch
from
43cb034
to
a67e93e
Compare
I understand. I think they currently pick a few random combinations of #define, and see how they go. The fact is that in some/all combinations of there, the LVFontGlyphCacheItem is one (with 1 or 2 others) we always get, so there must be something odd. |
|
Look at all those |
|
Locally, from the crengine checkout: ▹ clang-tidy crengine/src/lvfntman.cpp -- -Icrengine/include
3 errors generated.
Error while processing […]/base/thirdparty/kpvcrlib/crengine/crengine/src/lvfntman.cpp.
[…]/base/thirdparty/kpvcrlib/crengine/crengine/src/../include/lvfntman.h:148:25: error: 'LVFontGlyphCacheItem' does not refer to a value [clang-diagnostic-error]
148 | return offsetof(LVFontGlyphCacheItem, bmp) + ((bmp_width * bmp_height) * sizeof(*bmp));
| ^
[…]/base/thirdparty/kpvcrlib/crengine/crengine/src/../include/lvfntman.h:124:8: note: declared here
124 | struct LVFontGlyphCacheItem
| ^
[…]/base/thirdparty/kpvcrlib/crengine/crengine/src/../include/lvfntman.h:152:80: error: 'LVFontGlyphCacheItem' does not refer to a value [clang-diagnostic-error]
152 | LVFontGlyphCacheItem * item = (LVFontGlyphCacheItem *)malloc( offsetof(LVFontGlyphCacheItem, bmp)
| ^
[…]/base/thirdparty/kpvcrlib/crengine/crengine/src/../include/lvfntman.h:124:8: note: declared here
124 | struct LVFontGlyphCacheItem
| ^
[…]/base/thirdparty/kpvcrlib/crengine/crengine/src/../include/lvfntman.h:152:102: error: invalid use of member 'bmp' in static member function [clang-diagnostic-error]
152 | LVFontGlyphCacheItem * item = (LVFontGlyphCacheItem *)malloc( offsetof(LVFontGlyphCacheItem, bmp)
| ^~~
Found compiler error(s).
▹ clang-tidy crengine/src/lvfntman.cpp -- -DLINUX -Icrengine/include
1 error generated.
Error while processing […]/base/thirdparty/kpvcrlib/crengine/crengine/src/lvfntman.cpp.
[…]/base/thirdparty/kpvcrlib/crengine/crengine/src/../include/textlang.h:5:10: error: 'hb.h' file not found [clang-diagnostic-error]
5 | #include <hb.h>
| ^~~~~~
Found compiler error(s).It's the config. |
|
Good to go? Or still working on it? |
Remaining:
This code is pretty hairy… Interestingly, those warnings go away when disabling crengine's custom memory manager (
-DLDOM_USE_OWN_MEM_MAN=0).This change is