Open benoit-pierre opened 1 month ago
Looks like you did not touch anything related to LVFontGlyphCacheItem, so mentionning it - I see this one too often: https://github.com/koreader/crengine/actions/runs/10022158348/job/27701636550
Running clang-tidy on crengine/src/lvfntman.cpp
2 warnings and 3 errors generated.
Error while processing /home/runner/work/crengine/crengine/crengine/src/lvfntman.cpp.
/home/runner/work/crengine/crengine/crengine/src/../include/lvfntman.h:148:25: error: 'LVFontGlyphCacheItem' does not refer to a value [clang-diagnostic-error]
return offsetof(LVFontGlyphCacheItem, bmp) + ((bmp_width * bmp_height) * sizeof(*bmp));
^
Suppressed 2 warnings (2 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
Found compiler error(s).
/home/runner/work/crengine/crengine/crengine/src/../include/lvfntman.h:124:8: note: declared here
struct LVFontGlyphCacheItem
^
/home/runner/work/crengine/crengine/crengine/src/../include/lvfntman.h:152:80: error: 'LVFontGlyphCacheItem' does not refer to a value [clang-diagnostic-error]
LVFontGlyphCacheItem * item = (LVFontGlyphCacheItem *)malloc( offsetof(LVFontGlyphCacheItem, bmp)
^
/home/runner/work/crengine/crengine/crengine/src/../include/lvfntman.h:124:8: note: declared here
struct LVFontGlyphCacheItem
^
/home/runner/work/crengine/crengine/crengine/src/../include/lvfntman.h:152:102: error: invalid use of member 'bmp' in static member function [clang-diagnostic-error]
LVFontGlyphCacheItem * item = (LVFontGlyphCacheItem *)malloc( offsetof(LVFontGlyphCacheItem, bmp)
Let me do a pass with clang.
For the indentation issues: we compile with -ftabstop=4
, this take care of most warnings due to mixing both tabs and spaces to indent. The remaining warnings code looks like -ftabstop=8
is assumed (which is Github default too, I guess). I can either amend to use tabs, or spaces, everywhere to fix those and the Github display issue.
Regarding asserts: they are disabled in release mode, so good to shut up compiler warnings, detect coding/logic mistakes, possible bugs.
I can either amend to use tabs, or spaces, everywhere to fix the warning and the Github display issue.
"everywhere" meaning where there are warnings right ? :) (Keep changes minimal to avoid messing git blame). I'd say: do as I do, conform to the spacing used by the lines in the function you don't need to touch (and if between 2 styles/solutions, chose the one needing less changes, or changing dumb lines without much logic).
I can either amend to use tabs, or spaces, everywhere to fix the warning and the Github display issue.
"everywhere" meaning where there are warnings right ? :) (Keep changes minimal to avoid messing git blame). I'd say: do as I do, conform to the spacing used by the lines in the function you don't need to touch (and if between 2 styles/solutions, chose the one needing less changes, or changing dumb lines without much logic).
Yes, only to fix the warnings, minimal changes.
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 crsetup.h
file generated by the build system is used, and all the necessary headers (including dependencies) are available.
Maybe we should revisit the crsetup.h
situation. Currently, they are 3 defines set by cmake at configure time: _DEBUG
, DISABLE_CLOEXEC
& HAVE_OFF64_T
.
And those are used in crengine headers, so currently part of the public API.
_DEBUG
lvarray.h
:
/// copies range to beginning of array
void trim( int pos, int count, int reserved )
{
#if defined(_DEBUG) && !defined(ANDROID)
if ( pos<0 || count<=0 || pos+count > _count )
throw;
#endif
[…]
/// removes several items from vector
void erase( int pos, int count )
{
#if defined(_DEBUG) && !defined(ANDROID)
if ( pos<0 || count<=0 || pos+count > _count )
throw;
#endif
[…]
We could change this code to only detect coding mistakes:
/// copies range to beginning of array
void trim( int pos, int count, int reserved )
{
assert( pos >= 0 && count > 0 && pos+count <= _count );
[…]
/// removes several items from vector
void erase( int pos, int count )
{
assert( pos >= 0 && count > 0 && pos+count <= _count );
[…]
Which would please cppcheck:
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().
lvtinydom.h
:
#ifdef _DEBUG
#if BUILD_LITE!=1
///debug method, for DOM tree consistency check, returns false if failed
bool checkConsistency( bool requirePersistent );
#endif
#endif
All calls to checkConsistency
are commented out, so we can just comment the declaration too.
lvstring.h
:
#ifdef _DEBUG
#include <stdio.h>
class DumpFile
{
[…]
};
#endif
No change to other class/struct layouts, and DumpFile
is only used in wolutil.cpp
(which we don't compile anymore). We can comment that part.
And then move _DEBUG
out of crsetup.h
(add it to crengine's private compilations flags).
DISABLE_CLOEXEC
& HAVE_OFF64_T
That part is more thorny:
/// Streams.
#cmakedefine DISABLE_CLOEXEC 1
#cmakedefine HAVE_OFF64_T 1
#ifdef HAVE_OFF64_T
# define HAVE_STAT64 1
# define _LARGEFILE64_SOURCE 1
#endif
DISABLE_CLOEXEC
is used in lvplatform.h
, included by many, but it looks like the impact is limited to source files, so move it to the private compilation flags.
HAVE_OFF64_T
is not used anywhere else. HAVE_STAT64
is only used in lvstream.cpp
, but I don't know if _LARGEFILE64_SOURCE
can change the layout of some classes/structs (in lvstream.h
).
Assuming we can remove those #cmakedefines …
, to make crsetup.h
's contents static, we can replace the one in crengine
, and use it for clang-tidy/cppcheck and simplify a couple of things.
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.
No real thought / opinion about https://github.com/koreader/crengine/pull/579#issuecomment-2241796599 , so go ahead as you please :)
Btw, what about the LVFontGlyphCacheItem
warning I mentionned in 2nd post?
No real thought / opinion about #579 (comment) , so go ahead as you please :) Btw, what about the
LVFontGlyphCacheItem
warning I mentionned in 2nd post?
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 https://github.com/koreader/koreader-base/commit/941fbbf83bfbd179f2baeae9e03ba16c5aee1a72 locally (Arch Linux, clang-tidy 18.1.8) or in docker (using koreader/kobase:0.3.4-20.04
, clang-tidy 10.0.0).
The way the linters are run on GA does not match the actual build configuration.
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. Even if you can't reproduce it, do you understand what is the complain in the output on GA?
Look at all those Error while processing
and unknown type name 'uint64_t'
errors: parsing C/C++ is famously hard (context sensitive), and if the configuration is borked and some basic types are not recognized, then things can snowball from there.
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![Reviewable](https://reviewable.io/review_button.svg)